Skip to content

[Routing] Add locale requirement for localized routes #35692

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

Closed

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Feb 12, 2020

Q A
Branch? master
Bug fix? yes
New feature? no
Deprecations? no
License MIT

If you're using localized routes, you expect to have these kind of routes available:

  • /fr/accueil
  • /en/home

But nowadays, these routes are unexpectedly available:

  • /en/accueil
  • /fr/home

This PR proposes to add a strict locale requirement for localized so that the above routes won't be available.

Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

I would make this strictness opt-in, with a strict_i18n option defaulting to false for BC.
We should then trigger a deprecation that can be implemented in this PR, to default to true in Symfony 6.
If it happens that there are use cases for false within two years we can keep the option, otherwise deprecate it in 6.1 to remove it in 7.0.

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 13, 2020
@mtarld mtarld force-pushed the fix/localized-routes-requirements branch from 1c269ae to 8b3e8d0 Compare February 13, 2020 17:49
@mtarld mtarld marked this pull request as ready for review February 13, 2020 17:57
@nicolas-grekas
Copy link
Member

I'd prefer no options for this. Options increase complexity a lot.

@mtarld mtarld force-pushed the fix/localized-routes-requirements branch from 8b3e8d0 to 1520c4a Compare February 13, 2020 18:04
@nicolas-grekas
Copy link
Member

Should target 4.4 I'd say.

@mtarld mtarld changed the base branch from master to 4.4 February 15, 2020 12:28
@mtarld mtarld changed the base branch from 4.4 to master February 15, 2020 12:28
@mtarld
Copy link
Contributor Author

mtarld commented Feb 15, 2020

Should target 4.4 I'd say.

I agree, but this implementation is based on src/Symfony/Component/Routing/Loader/Configurator/Traits/LocalizedRouteTrait.php which isn't present in 4.4 (added in de74794)

Should I create another PR ?

@nicolas-grekas
Copy link
Member

Yes please

Co-authored-by: Mathias Arlaud <mathias.arlaud@gmail.com>
Co-authored-by: Ahmed Tailouloute <ahmed.tailouloute@gmail.com>
@nicolas-grekas
Copy link
Member

Closing in favor of #35735 actually.

fabpot added a commit that referenced this pull request Feb 20, 2020
…rld)

This PR was merged into the 4.4 branch.

Discussion
----------

[Routing] Add locale requirement for localized routes

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| License       | MIT

4.4 version of #35692

If you're using localized routes, you expect to have these kind of routes available:
- `/fr/accueil`
- `/en/home`

But nowadays, these routes are unexpectedly available:
- `/en/accueil`
- `/fr/home`

When importing routes like that:
- `prefix: "/{_locale}"`
- `@Route({"en": "/home", "fr": "/accueil"}, name="home")`

This PR proposes to add a strict locale requirement for localized so that the above routes won't be available.

Commits
-------

50d7445 [Routing] Add locale requirement for localized routes
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.

4 participants