-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Routing] Support UTF-8 in paths and parameters #19562
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
Some of the tests for FrameworkBundle are failing on PHP 7, because the tests assumes the changes made to symfony/routing by this PR, but Travis installs latest released version of symfony/routing. I can bump the requirement in composer.json, or I can make the code and tests compatible with both versions. How do we usually deal with this? |
@@ -82,11 +89,12 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt | |||
* @param RequestContext $context The context | |||
* @param LoggerInterface|null $logger A logger instance | |||
*/ | |||
public function __construct(RouteCollection $routes, RequestContext $context, LoggerInterface $logger = null) | |||
public function __construct(RouteCollection $routes, RequestContext $context, LoggerInterface $logger = null, $charset = 'UTF-8') |
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.
forgot to upgrade the docblock with new arg
@MacDada Thanks for the review. I have addresses all your comments. I still need some advice on the FrameworkBundle issue mentioned above. |
I think there is a mismatch is the definition of the -> we need a parameter to configure this fallback input charset. This can't be kernel.charset by default, but CP1252. Then there is the output encoding of the URLs we generate. These should match the output encoding the app is using. -> this matches the definition of the kernel.charset parameter so it can be injected here.
|
True, a final URL has no encoding. The charset is used when we want to represent a non-ASCII character. For example, the Wikipedia article about the letter É has the URL If ISO-8859-1 was used note (that was very common years ago), the URL would have been
That rule only comes into play if the HTML contains URLs containing characters that are not properly percent-encoded. Such URLs are not valid and should never occur in HTML generated by Symfony functions. However, since browsers prefer UTF-8 in the URL, I guess you would probably prefer to use UTF-8 as |
Not at all in my understanding, and in fact you cited what I meant: type
Yep, we need a fallbak input charset |
Which part of the HTML5 spec are you referring to? I guess we could implement a fallback encoding that automatically redirects like Wikipedia does, but I think that is a new feature that is outside the scope of this PR. |
an older draft one looks like, I can't find the reference :) |
I tried an alternative approach in #19604 |
This PR was merged into the 2.7 branch. Discussion ---------- [Routing] Add missing options in docblock | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Courtesy of @c960657 in #19562 Commits ------- f45da32 [Routing] Add missing options in docblock
Yes, let’s continue in #19604. |
…s (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [Routing] Add seamless support for unicode requirements | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #3629, #5236, #19562 | License | MIT | Doc PR | symfony/symfony-docs#6890 This PR adds unicode support to route matching and generation by automatically adding the `u` modifier to regexps that use either unicode characters or unicode enabled character classes (e.g. `\p...` `\x{...}` `\X`). As a side note, if one wants to match a single unicode character (vs a single byte), one should use `\PM` or `\X` instead of `.` *or* set the `unicode` parameter to true. Commits ------- a829d34 [Routing] Add seamless support for unicode requirements
This PR is a new take on a previous proposal in #19054.