-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Routing] Deal with hosts per locale #36187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d5e5146
to
e48a5b8
Compare
What happens when the route defines both localized paths and localized hosts?
I'm not sure this is correct: other attributes (defaults, condition, etc) give precedence to the importing side, isn't it? |
Both will be used. I didn't add any test for this case cause TBH I don't know what's the expected behavior.
You tell me. :) I can remove this behavior (here) if you think it makes sense. |
can you please add one? the behavior should be that we synchronize both path and host to create only one route for the locale, that matches host and path, both at the same time.
I confirm, see this line that already has the correct precedence. It would make no sense to have precedence vary by type of declaration I think. |
e48a5b8
to
3d8ccf8
Compare
This comment has been minimized.
This comment has been minimized.
e71c8b9
to
79942b2
Compare
src/Symfony/Component/Routing/Loader/Configurator/Traits/LocalizedRouteTrait.php
Show resolved
Hide resolved
Looks like all implementations (xml, yaml & php) are working now. :) Now waiting for your reviews! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice. Here are some questions + suggestions.
src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/Configurator/Traits/LocalizedRouteTrait.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/Configurator/Traits/HostTrait.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/Configurator/Traits/HostTrait.php
Outdated
Show resolved
Hide resolved
foreach ($host as $locale => $localeHost) { | ||
$localizedRoute = clone $route; | ||
$localizedRoute->setDefault('_locale', $locale); | ||
$localizedRoute->setRequirement('_locale', $locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing preg_quote()
btw, don't we miss this line in PrefixTrait
? if confirmed, the fix should be for 4.4
src/Symfony/Component/Routing/Loader/Configurator/Traits/HostTrait.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sweet thanks
Just one thing: can you please rebase and update the changelog of the component? |
ed39ce8
to
a1de9c4
Compare
Done! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, made a minor comment on the CHANGELOG entry.
a1de9c4
to
817a1af
Compare
Documentation available here: symfony/symfony-docs#13571 |
Thank you @odolbeau. |
Hello @odolbeau , Could it work with the subdomain system? With the files below, will it be possible to have
Thanks :) Here are my different files: # config/routes/annotations.yaml
controllers:
resource: ../../src/Controller/
type: annotation
host:
fr: www.example.fr
en: www.example.com // src/Controller/BlogController.php
namespace App\Controller;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Routing\Annotation\Route;
class BlogController extends AbstractController
{
/**
* @Route("/blog", name="blog_list")
*/
public function list()
{
// ...
}
} // src/Controller/Admin/BlogController.php
namespace App\Controller\Admin;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Routing\Annotation\Route;
class BlogController extends AbstractController
{
/**
* @Route("/", name="admin_index", host="admin")
*/
public function index()
{
// ...
}
} |
This PR was squashed before being merged into the 5.2 branch. Discussion ---------- [Routing] Explain host per locale <!-- If your pull request fixes a BUG, use the oldest maintained branch that contains the bug (see https://symfony.com/releases for the list of maintained branches). If your pull request documents a NEW FEATURE, use the same Symfony branch where the feature was introduced (and `5.x` for features of unreleased versions). --> Reopens #13571 initially opened by `@odolbeau` but staled for a while then closed when removing master branch. This is just a copy/paste from the inital PR. Documents feature: symfony/symfony#36187 Closes #13572 Commits ------- ebb266c [Routing] Explain host per locale
Allow to define a different host for each locale in routing.
It's now possible to define this kind of configuration:
It's still possible to define an unique host (
host: wwww.example.com
) and if a host is defined for a given route directly, it's not overridden.To be done: