8000 [Routing] UTF-8 support for parameter requirements by c960657 · Pull Request #19054 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] UTF-8 support for parameter requirements #19054

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.

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
Closed
wants to merge 3,296 commits into from
Closed

[Routing] UTF-8 support for parameter requirements #19054

wants to merge 3,296 commits into from

Conversation

c960657
Copy link
Contributor
@c960657 c960657 commented Jun 14, 2016 •
edited
Loading
Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Assume I have a route /{foo} and a requirement \pL. I would expect this to match the request URL /%C3%A9 (URL-encoded version of /é), but it doesn't.

In UrlGenerator::doGenerate() it is generally assumed that the variables in $mergedParams are UTF-8-encoded (because they are passed to rawurlencode() that escapes each octet in the UTF-8-encoded string as described in RFC 3986). However, the requirement checkout using preg_match() does not use the u modifier, so the requirement . does not match the above URL, but .. does. In other words, the pattern should match the bytes in the UTF-8 encoded string, not the string itself (so to speak).

This patch adds the u modifier but preserves backwards compatibility with patterns made that circumvents the current limitation. In the master branch, the second preg_match() should be omitted.

This patch is slightly more liberal in validating parameter requirements, so it may break BC in some edge cases if people have worked around the existing limitation.

I know that the Routing component does not claim to be fully UTF-8 compliant, but this a least takes it a bit closer.

@Tobion
Copy link
Contributor
Tobion commented Jun 14, 2016

Ref #5236

You changed the UrlGenerator to use the u modifier for regexes but the UrlMatcher/Compiler still doesn't use the u modifer, see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/RouteCompiler.php#L166
So that seems inconsistent and buggy now because you could generate a URL that doesn't match.

@Tobion
Copy link
Contributor
Tobion commented Jun 14, 2016

Status: Needs Work

@nicolas-grekas
Copy link
Member

Adding u to the router matcher would break URLs that are not encoded as UTF-8, which is perfectly legal. How should we deal with that?

@c960657
Copy link
Contributor Author
c960657 commented Jun 15, 2016

@Tobion You are right. I'll look into that also.

@nicolas-grekas I suggest we check both variants similarly to hos this PR does in the generator, possibly depending on the value of the default_charset or input_encoding ini setting or some explicit configuration setting on the router.

You are right that non-UTF-8 URLs are valid. However, supporting such URLs with Symfony seems like an uphill struggle. AFAICT only UTF-8 is supported for the static parts of the routes when using the Yaml and XML file format (because the Component\Yaml and PHP's DOM extension always return UTF-8-encoded strings). Browsers seems to assume that URL's are UTF-8-encoded when pretty-printing URLs e.g. in the address field. So I wonder whether this is worth supporting. Does Symfony really work well with other encodings?

If we decide to drop support for this in future versions, we could still detect non-UTF-8 strings in incoming requests and convert them to UTF-8 accordingly.

Anyway, dropping support for non-UTF-8 is not necessary in order to accomplish the goals of this PR.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jun 15, 2016 •
edited
Loading

@c960657 IMHO we should not auto-magically deal with charsets. Let's add an option to enable UTF-8 on routes, either on individual routes or globally on the app, and add the u modifier accordingly.

fabpot and others added 20 commits June 28, 2016 18:24
@fabpot
Merge branch '2.7' into 2.8
6b81f2c 
* 2.7:
  [CS] Respect PSR2 4.2
  [Form] fix `empty_data` option in expanded `ChoiceType`
  [Console] removed unneeded private methods
  sync min email validator version
  [TwigBridge] Fix inconsistency in LintCommand help
  explicitly forbid e-mail validator 2.0 or higher
  Fixed SymfonyQuestionHelper multi-choice with defaults
  [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure
  Differentiate between the first time a progress bar is displayed and subsequent times
  finished previous commit
  No more exception for malformed input name
  fix post_max_size_message translation
  [Process] Fix pipes cleaning on Windows
  Avoid phpunit 5.4 warnings on getMock
  [Form] Add exception to FormRenderer about non-unique block names
  [Form] Consider a violation even if the form is not submitted
@fabpot
Merge branch '2.8' into 3.0
ba84160 
* 2.8:
  [CS] Respect PSR2 4.2
  [Form] fix `empty_data` option in expanded `ChoiceType`
  [Console] removed unneeded private methods
  [Security] [Guard] Improve comment with working example
  sync min email validator version
  [TwigBridge] Fix inconsistency in LintCommand help
  explicitly forbid e-mail validator 2.0 or higher
  Fixed SymfonyQuestionHelper multi-choice with defaults
  [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure
  Differentiate between the first time a progress bar is displayed and subsequent times
  finished previous commit
  No more exception for malformed input name
  fix post_max_size_message translation
  [Process] Fix pipes cleaning on Windows
  Avoid phpunit 5.4 warnings on getMock
  [Form] Add exception to FormRenderer about non-unique block names
  [Form] Consider a violation even if the form is not submitted
@fabpot
Merge branch '3.0' into 3.1
45ce2c8 
* 3.0:
  [CS] Respect PSR2 4.2
  [Form] fix `empty_data` option in expanded `ChoiceType`
  [Console] removed unneeded private methods
  [Security] [Guard] Improve comment with working example
  sync min email validator version
  [TwigBridge] Fix inconsistency in LintCommand help
  explicitly forbid e-mail validator 2.0 or higher
  Fixed SymfonyQuestionHelper multi-choice with defaults
  [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure
  Differentiate between the first time a progress bar is displayed and subsequent times
  finished previous commit
  No more exception for malformed input name
  fix post_max_size_message translation
  [Process] Fix pipes cleaning on Windows
  Avoid phpunit 5.4 warnings on getMock
  [Form] Add exception to FormRenderer about non-unique block names
  [Form] Consider a violation even if the form is not submitted
@tucksaun
[Security] Allow LDAP loadUser override
ae99aa8
@fabpot
Merge branch '3.1'
3c47306 
* 3.1:
  [CS] Respect PSR2 4.2
  [Form] fix `empty_data` option in expanded `ChoiceType`
  [Console] removed unneeded private methods
  updated Http-Kernel dependency
  [Security] [Guard] Improve comment with working example
  sync min email validator version
  [TwigBridge] Fix inconsistency in LintCommand help
  explicitly forbid e-mail validator 2.0 or higher
  Fixed SymfonyQuestionHelper multi-choice with defaults
  [DoctrineBridge] Don't use object IDs in DoctrineChoiceLoader when passing a value closure
  Differentiate between the first time a progress bar is displayed and subsequent times
  finished previous commit
  No more exception for malformed input name
  fix post_max_size_message translation
  [Process] Fix pipes cleaning on Windows
  Avoid phpunit 5.4 warnings on getMock
  [Form] Add exception to FormRenderer about non-unique block names
  [FrameworkBundle] templating can be fully disabled
  [Form] Consider a violation even if the form is not submitted
@fabpot
fixed typo
f4dbd30
@fabpot
Merge branch '2.8' into 3.0
c40bcd8 
* 2.8:
  fixed typo
@fabpot
Merge branch '3.0' into 3.1
cef1915 
* 3.0:
  fixed typo
@fabpot
Merge branch '3.1'
764228f 
* 3.1:
  fixed typo
@Tobion
[Form] fix post max size translation type extension for >= 2.8
a27ec00
@chalasr
[Console] Fix formatting of SymfonyStyle::comment()
0a53e1d 
Remove decoration from frameworkbundle test (avoid testing the Console behaviour)
Set background to default

Test output

Adapt test for FrameworkBundle

Use Helper::strlenWithoutDecoration rather than Helper::strlen(strip_tags(..))

Improve logic for align all lines to the first in block()

Tests more block() possible outputs

Avoid calling Helper::strlenWithoutDecoration in loop for prefix, assign it instead
@fabpot
bug symfony#19211 [Form] fix post max size translation type extension…
c68508f 
… for >= 2.8 (Tobion)

This PR was merged into the 2.8 branch.

Discussion
----------

[Form] fix post max size translation type extension for >= 2.8

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? |no
| Tests pass?   | yes
| Fixed tickets | symfony#19210, symfony#19061
| License       | MIT
| Doc PR        |

Commits
-------

a27ec00 [Form] fix post max size translation type extension for >= 2.8
@fabpot
fixed form tests
d0130d9
@fabpot
bug symfony#19189 [Console] Fix formatting of SymfonyStyle::comment()…
c278399 
… (chalasr)

This PR was merged into the 2.8 branch.

Discussion
----------

[Console] Fix formatting of SymfonyStyle::comment()

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#19172
| License       | MIT
| Doc PR        | n/a

This:

```php
$io->comment('Lorem ipsum dolor sit amet, consectetur adipisicing elit, <comment>sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat </comment>cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.');
```

Before outputs:

![](http://image.prntscr.com/image/1d2ea9de42024b53a77120c482be51d4.png)

After:

![](http://image.prntscr.com/image/36de23ec14b64804b0cbae7a431185be.png)

This moves the lines-cutting logic from `block()` into a specific `createBlock`, used from both `comment()` and `block()`, sort as `comment()` can take messages containing nested tags and outputs a correctly formatted block without escaping tags.

Commits
-------

0a53e1d [Console] Fix formatting of SymfonyStyle::comment()
@ro0NL @fabpot
[Workflow] CS tweaks
5428a92
@fabpot
minor symfony#19187 [Workflow] CS tweaks (ro0NL)
f720444 
This PR was squashed before being merged into the 3.2-dev branch (closes symfony#19187).

Discussion
----------

[Workflow] CS tweaks

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

Just a few minor tweaks/fixes after playing around with it. Some phpdoc should still be added imo, like missing `@throws`, right?

Commits
-------

5428a92 [Workflow] CS tweaks
@fabpot
bug symfony#19204 [Security] Allow LDAP loadUser override (tucksaun)
7b2af2d 
This PR was merged into the 3.1 branch.

Discussion
----------

[Security] Allow LDAP loadUser override

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

Back to 3.0, one could extend `Symfony\Component\Security\Core\User\LdapUserProvider` and override how User objects are created.
Among several improvements, symfony#17560 changed `loadUser` signature but also visibility to `private` which disallow any overriding.
Even if the signature BC break is legitimate, we should still be able to override this method IMHO, which is not possible with a private visibility.
This PRs introduces a `protected` visibility to allow again overriding.

Commits
-------

ae99aa8 [Security] Allow LDAP loadUser override
@fabpot
minor symfony#19213 fixed form tests (fabpot)
fd24e0a 
This PR was merged into the 2.8 branch.

Discussion
----------

fixed form tests

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

d0130d9 fixed form tests
@fabpot
Merge branch '2.7' into 2.8
bd1e3e3 
* 2.7:
  removed dots at the end of @param and @return
  fixed typo
@fabpot
fixed CS
6271446
fabpot and others added 20 commits July 30, 2016 10:48
@fabpot
Merge pull request symfony#19487 from fabpot/release-2.8.9
df02dd5 
released v2.8.9
@fabpot
bumped Symfony version to 2.8.10
068b0f1
@fabpot
updated CHANGELOG for 3.1.3
bc60a7f
@fabpot
updated VERSION for 3.1.3
01ece68
@fabpot
Merge pull request symfony#19489 from fabpot/release-3.1.3
4478f04 
released v3.1.3
@fabpot
bumped Symfony version to 3.1.4
be4255e
@xabbuh
deprecate missing space after mapping key colon
9a31eef
@fabpot
feature symfony#19430 [DomCrawler] Add support for XPath expression e…
a0b22f0 
…valuation (jakzal)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[DomCrawler] Add support for XPath expression evaluation

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

Example usage:

```php
<?php

use Symfony\Component\DomCrawler\Crawler;
use Symfony\Component\VarDumper\VarDumper;

require_once __DIR__.'/vendor/autoload.php';

$html = '<html>
<body>
    <span id="article-100" class="article">Article 1</span>
    <span id="article-101" class="article">Article 2</span>
    <span id="article-102" class="article">Article 3</span>
</body>
</html>';

$crawler = new Crawler();
$crawler->addHtmlContent($html);

VarDumper::dump($crawler->filterXPath('//span[contains(@id, "article-")]')->evaluate('substring-after(@id, "-")'));
// array:3 [
//   0 => "100"
//   1 => "101"
//   2 => "102"
// ]

VarDumper::dump($crawler->evaluate('substring-after(//span[contains(@id, "article-")]/@id, "-")'));
// array:1 [
//   0 => "100"
// ]

VarDumper::dump($crawler->filterXPath('//span[@Class="article"]')->evaluate('count(@id)'));
// array:3 [
//   0 => 1.0
//   1 => 1.0
//   2 => 1.0
// ]

VarDumper::dump($crawler->evaluate('count(//span[@Class="article"])'));
// array:1 [
//   0 => 3.0
// ]

VarDumper::dump($crawler->evaluate('//span[1]'));
// Symfony\Component\DomCrawler\Crawler { }
```

Commits
-------

3148fad [DomCrawler] Add support for XPath expression evaluation
@fabpot
feature symfony#19504 [Yaml] deprecate missing space after mapping ke…
e408b50 
…y colon (xabbuh)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Yaml] deprecate missing space after mapping key colon

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

Commits
-------

9a31eef deprecate missing space after mapping key colon
@nicolas-grekas
[Cache] Minor cleanup
34d9518
@nicolas-grekas
Merge branch '2.7' into 2.8
4ebe440 
* 2.7:
  Remove usage of __CLASS__ outside of a class
  [HttpKernel] Fix variable conflicting name
  [Process] Fix double-fread() when reading unix pipes
  [Process] Fix AbstractPipes::write() for a situation seen on HHVM (at least)
  [Validator] Fix dockblock typehint in XmlFileLoader
  bumped Symfony version to 2.7.17
  updated VERSION for 2.7.16
  update CONTRIBUTORS for 2.7.16
  updated CHANGELOG for 2.7.16

Conflicts:
	src/Symfony/Component/HttpKernel/Kernel.php
@fabpot @remicollet
bumped Symfony version to 2.8.10
eabbcf0
@remicollet
Relax 1 test failing with latest PHP versions
6703b41 
Related to php bug symfony#52646 which is fixed in 5.6.25RC1, 7.0.10RC1, 7.1.0beta2
@nicolas-grekas
minor symfony#19539 [VarDumper] Relax 1 test failing with latest PHP …
2f9921f 
…versions (fabpot, remicollet)

This PR was merged into the 2.8 branch.

Discussion
----------

[VarDumper] Relax 1 test failing with latest PHP versions

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | no
| New feature?  | o
| BC breaks?    |  no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT
| Doc PR        | reference to the documentation PR, if any

Related to php bug https://bugs.php.net/72646 which is fixed in 5.6.25RC1, 7.0.10RC1, 7.1.0beta2

Detected in Fedora CI, failed since 7.0.10RC1, see
https://apps.fedoraproject.org/koschei/package/php-symfony

Commits
-------

6703b41 Relax 1 test failing with latest PHP versions
eabbcf0 bumped Symfony version to 2.8.10
@nicolas-grekas
Merge branch '2.8' into 3.1
6f390d4 
* 2.8:
  Relax 1 test failing with latest PHP versions
  bumped Symfony version to 2.8.10
  Remove usage of __CLASS__ outside of a class
  [HttpKernel] Fix variable conflicting name
  [Process] Fix double-fread() when reading unix pipes
  [Process] Fix AbstractPipes::write() for a situation seen on HHVM (at least)
  [Validator] Fix dockblock typehint in XmlFileLoader
  bumped Symfony version to 2.8.10
  updated VERSION for 2.8.9
  updated CHANGELOG for 2.8.9
  bumped Symfony version to 2.7.17
  updated VERSION for 2.7.16
  update CONTRIBUTORS for 2.7.16
  updated CHANGELOG for 2.7.16
  Minor fixes
  [Console] Overcomplete argument exception message tweak.
  fixed bad auto merge
  Console table cleanup
  undefined offset fix (symfony#19406)
  [EventDispatcher] Removed unused variable

Conflicts:
	CHANGELOG-2.7.md
	CHANGELOG-3.0.md
	src/Symfony/Bridge/Swiftmailer/DataCollector/MessageDataCollector.php
	src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
	src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
	src/Symfony/Component/Console/Tests/Helper/LegacyDialogHelperTest.php
	src/Symfony/Component/Console/Tests/Helper/TableTest.php
	src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/legacy-container9.php
	src/Symfony/Component/EventDispatcher/Tests/AbstractEventDispatcherTest.php
	src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/LegacyPdoSessionHandlerTest.php
	src/Symfony/Component/HttpKernel/Kernel.php
@nicolas-grekas
minor symfony#19540 [Cache] Minor cleanup (nicolas-grekas)
c45b916 
This PR was merged into the 3.1 branch.

Discussion
----------

[Cache] Minor cleanup

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

Commits
-------

34d9518 [Cache] Minor cleanup
@nicolas-grekas
Merge branch '3.1'
973f295 
* 3.1:
  Relax 1 test failing with latest PHP versions
  bumped Symfony version to 2.8.10
  [Cache] Minor cleanup
  Remove usage of __CLASS__ outside of a class
  [HttpKernel] Fix variable conflicting name
  [Process] Fix double-fread() when reading unix pipes
  [Process] Fix AbstractPipes::write() for a situation seen on HHVM (at least)
  [Validator] Fix dockblock typehint in XmlFileLoader
  bumped Symfony version to 3.1.4
  updated VERSION for 3.1.3
  updated CHANGELOG for 3.1.3
  bumped Symfony version to 2.8.10
  updated VERSION for 2.8.9
  updated CHANGELOG for 2.8.9
  bumped Symfony version to 2.7.17
  updated VERSION for 2.7.16
  update CONTRIBUTORS for 2.7.16
  updated CHANGELOG for 2.7.16

Conflicts:
	src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
	src/Symfony/Component/HttpKernel/Kernel.php
@nicolas-grekas
[Cache] Skip tests that sleep() but can't be clock-mocked
d476725
@nicolas-grekas
minor symfony#19544 [Cache] Skip tests that sleep() but can't be cloc…
49a2cb8 
…k-mocked (nicolas-grekas)

This PR was merged into the 3.1 branch.

Discussion
----------

[Cache] Skip tests that sleep() but can't be clock-mocked

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

Skip tests that require real calls to `sleep()`: they slow down the test suite too much and don't test much.
`@group time-sensitive` tests will run these test cases just fine, but they can't be used on tests that use an external source for time, i.e. for redis or apcu.

Commits
-------

d476725 [Cache] Skip tests that sleep() but can't be clock-mocked
@nicolas-grekas
Merge branch '3.1'
64ace10 
* 3.1:
  [Cache] Skip tests that sleep() but can't be clock-mocked
@c960657 c960657 force-pushed the utf8-routes branch 4 times, most recently from b63c6a7 to be17ac7 Compare August 7, 2016 16:22
@c960657 c960657 mentioned this pull request Aug 7, 2016
Closed
@c960657
Copy link
Contributor Author
c960657 commented Aug 7, 2016

I have made a new PR against the master branch: #19562

The default encoding is UTF-8. It can be changed with an application-wide setting if one needs to support legacy URLs (I can't think of other reasons for not using UTF-8).

@c960657 c960657 closed this Aug 7, 2016
@c960657 c960657 changed the base branch from 2.7 to master September 28, 2016 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Routing Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0