Skip to content
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

[Routing] Deal with hosts per locale #36187

Merged
merged 1 commit into from Apr 20, 2020
Merged

Conversation

odolbeau
Copy link
Contributor

@odolbeau odolbeau commented Mar 24, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #30617
License MIT
Doc PR TODO

Allow to define a different host for each locale in routing.

It's now possible to define this kind of configuration:

controllers:
    resource: ../../src/Controller/
    type: annotation
    host:
        fr: www.example.fr
        en: www.example.com

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:

  • YamlLoader
  • XmlLoader
  • PhpLoader?
  • Documentation
  • Changelog

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 31, 2020

What happens when the route defines both localized paths and localized hosts?

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.

I'm not sure this is correct: other attributes (defaults, condition, etc) give precedence to the importing side, isn't it?

@odolbeau
Copy link
Contributor Author

odolbeau commented Mar 31, 2020

What happens when the route defines both localized paths and localized hosts?

Both will be used. I didn't add any test for this case cause TBH I don't know what's the expected behavior.

I'm not sure this is correct: other attributes (defaults, condition, etc) give precedence to the importing side, isn't it?

You tell me. :) I can remove this behavior (here) if you think it makes sense.
I added it cause you may want to define a totally different domain for a given route (and only for this one).
Anyway, it's doable by adding another rule in configuration so giving precedence to the importing side is not a problem.

@nicolas-grekas
Copy link
Member

I didn't add any test for this case cause TBH I don't know what's the expected behavior.

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 can remove this behavior (here) if you think it makes sense.

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.

@odolbeau

This comment has been minimized.

@odolbeau odolbeau force-pushed the array-of-hosts branch 4 times, most recently from e71c8b9 to 79942b2 Compare April 4, 2020 14:42
@odolbeau
Copy link
Contributor Author

odolbeau commented Apr 4, 2020

Looks like all implementations (xml, yaml & php) are working now. :)
For tests, I use a route file container 3 routes: 1 without host nor path, 1 with a single host, 1 with 2 hosts & 2 paths.
This file is imported 4 times: without any host, with only 1 host, with several hosts & with both hosts & locales (prefixes)
I tried to be as exhaustive as possible for those tests, tell me if I forget something.

Now waiting for your reviews! :)

@welcoMattic
Copy link
Member

Thanks @odolbeau for your work!
I tested it here with a replication of a real-life case based on a client project, and it works like a charm 👌

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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/XmlFileLoader.php Outdated Show resolved Hide resolved
src/Symfony/Component/Routing/Loader/XmlFileLoader.php Outdated Show resolved Hide resolved
src/Symfony/Component/Routing/Loader/XmlFileLoader.php Outdated Show resolved Hide resolved
foreach ($host as $locale => $localeHost) {
$localizedRoute = clone $route;
$localizedRoute->setDefault('_locale', $locale);
$localizedRoute->setRequirement('_locale', $locale);
Copy link
Member

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

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet thanks

@nicolas-grekas
Copy link
Member

Just one thing: can you please rebase and update the changelog of the component?

@odolbeau
Copy link
Contributor Author

Just one thing: can you please rebase and update the changelog of the component?

Done! 👍

Copy link
Member

@fabpot fabpot left a 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.

src/Symfony/Component/Routing/CHANGELOG.md Outdated Show resolved Hide resolved
@odolbeau
Copy link
Contributor Author

Documentation available here: symfony/symfony-docs#13571

@fabpot
Copy link
Member

fabpot commented Apr 20, 2020

Thank you @odolbeau.

@fabpot fabpot merged commit e464954 into symfony:master Apr 20, 2020
@odolbeau odolbeau deleted the array-of-hosts branch April 20, 2020 08:56
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
@seb-jean
Copy link
Contributor

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()
    {
        // ...
    }
}

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 21, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Routing] Implement host per locale
6 participants