[Routing] Replace previously added preg_match with preg_replace_callback #18424
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ping @Tobion |
|
http://achelois.tweakers.net/~acm/urlrawencode_or_preg_replace.php is not reachable. |
| @@ -28,6 +28,30 @@ | |||
| class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInterface | |||
| { | |||
| /** | |||
| * This regexp matches all characters that should be percent encoded in paths for url's generated by this class. | |||
This argument is irrelevant as every extension point can be misused. |
| } elseif (preg_match($this->urlEncodingSkipRegexp, $url)) { | ||
| // the context base URL is already encoded (see Symfony\Component\HttpFoundation\Request) | ||
| } elseif (!empty($this->decodedChars)) { | ||
| // Support old style of character replacement, for when someone changed the decodedChars-array, |
There was a problem hiding this comment.
please remove this comment as it's obvious given the deprecation
|
I fixed that server, so achelois.tweakers.net should be available again. |
|
@Tobion I think I got all your comments |
|
Btw, I also ran your benchmarks against PHP 7 and it showed a performance decrease. So IMO there is no consistently better way than the current approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change was originally provided via #18379, but it was considered a BC-breakage due to the deprecation of the decodedChars-parameter on the UrlGenerator-class.
Please note that while that parameter could be seen as an extension point, it was also prone to errors in routing: by altering that array the UrlGenerator's output was not guaranteed to be RFC-safe or to match the UrlMatcher's expectations. I.e. one could set %2F to be converted to \ rather than /, completely breaking url-matching.
This PR is to remove the array, it was initially intended for its performance gain. But it also yields safer url generation (see before) and has more logical code. The old code percent-encoded (almost) all chars, but reverted some to their original character.
By replacing it with a regexp, we can inverse the behaviour of the 'rawurlencode, than decode "wanted chars" back via strtr' into 'only rawurlencode the unwanted chars'. This gives a small (in absolute terms) performance gain, since strtr is fairly slow. Due to its high relative gain it starts to adds up when you generate many urls; in the tests quoted in #18257 (comment) it'd gain about 0,5ms for every 100 url's.
While that is a minor absolute number, for pages (like ours) that generally complete within 50ms, that's still a 1% gain by simply having faster url generation.
See this test script as well: http://achelois.tweakers.net/~acm/urlrawencode_or_preg_replace.php
The not-empty test of the BC-addition will probably counter part of that gain, though.