8000 [FrameworkBundle] allow turning routes to utf8 mode by default by nicolas-grekas · Pull Request #27774 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] allow turning routes to utf8 mode by default #27774

New iss 8000 ue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 9, 2018
Merged

[FrameworkBundle] allow turning routes to utf8 mode by default #27774

merged 1 commit into from
Jul 9, 2018

Conversation

nicolas-grekas
Copy link
Member
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This allows building optimized routers that match in a single regexp instead of an alternate of utf8/non-utf8 set of routes.

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 29, 2018
@stof
Copy link
Member
stof commented Jun 29, 2018

what is the impact of enabling utf8 for all routes ? Would they match different things ?

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Jun 29, 2018 •
edited
Loading

A URL containing above-ASCII chars presented in a non-UTF8 charset by a browser would be 404 where it would not necessarily today. But when the app is configured with kernel.charset=UTF-8, I fail to see any practical use case since nobody can know in which charset it is really. As such it would be hard to map to any actual content/resource...

@javiereguiluz
Copy link
Member

I agree with @stof. This looks absolutely reasonable and the right thing to do 👍 ... but I can't imagine the consequences (if any): would any route matching change? would routing performance be lower? etc.

@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] set routes in utf8 mode when kernel.charset=UTF-8 [FrameworkBundle] allow turning routes to utf8 mode by default Jul 5, 2018
@nicolas-grekas
Copy link
Member Author

To be extra safe, I added a new config option to enable utf8 mode, off by default, with a note in UPGRADE-4.2 to invite to turn it on.

@nicolas-grekas nicolas-grekas mentioned this pull request Jul 6, 2018
Merged
@fabpot
Copy link
Member
fabpot commented Jul 9, 2018

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 8f359cc into symfony:master Jul 9, 2018
fabpot added a commit that referenced this pull request Jul 9, 2018
@fabpot
feature #27774 [FrameworkBundle] allow turning routes to utf8 mode by…
254f4c8 
… default (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[FrameworkBundle] allow turning routes to utf8 mode by default

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This allows building optimized routers that match in a single regexp instead of an alternate of utf8/non-utf8 set of routes.

Commits
-------

8f359cc [FrameworkBundle] allow turning routes to utf8 mode by default
@nicolas-grekas nicolas-grekas deleted the route-default-utf8 branch July 22, 2018 20:25
@javiereguiluz javiereguiluz mentioned this pull request Sep 7, 2018
Closed
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
Closed
Merged
@niklasnatter niklasnatter mentioned this pull request Oct 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Deprecation Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@nicolas-grekas @stof @javiereguiluz @fabpot @carsonbot
0