[Routing] Replace previously added preg_match with preg_replace_callback #18379
Conversation
| protected $decodedChars = array( | ||
| // the slash can be used to designate a hierarchical structure and we want allow using it with this meaning | ||
| // some webservers don't allow the slash in encoded form in the path for security reasons anyway | ||
| // see http://stackoverflow.com/questions/4069002/http-400-if-2f-part-of-get-url-in-jboss |
There was a problem hiding this comment.
I think this was designed as "extension point" and removing a protected property is considered a BC break and will (sadly) break things. I just googled a bit and it looks like Drupal uses this for example: https://github.com/drupal/drupal/blob/8.1.x/core/lib/Drupal/Core/Routing/UrlGenerator.php#L62
There was a problem hiding this comment.
their code is broken anyway, as they replace a map with a list
There was a problem hiding this comment.
actually, they also replace the code using the property. So this change will not break Drupal (but Drupal itself should not reuse the Symfony property name for an incompatible purpose)
There was a problem hiding this comment.
I agree that this is not the best way of doing so, but it's still a way that people used to extend upon: http://stackoverflow.com/questions/24085126/patching-extending-route-urlgenerator-for-url-rewriting-to-replace-all-spaces-wi
It shouldn't be hard to create a BC layer though, if (!empty($this->decodedChars)) { /* @trigger_error(...).
There was a problem hiding this comment.
btw, the fact that they copy/paste the Symfony code looks weird to me (especially if the behavior does not match)
There was a problem hiding this comment.
Ok, I restored the decodedChars as @iltar suggested, how do I proceed from here? Should I also add a patch to 3.1, to remove the decodedChars entirely?
| } elseif (preg_match($this->urlEncodingSkipRegexp, $url)) { | ||
| // the context base URL is already encoded (see Symfony\Component\HttpFoundation\Request) | ||
| } elseif (!empty($this->decodedChars)) { | ||
| @trigger_error('The class variable '.__CLASS__.'::$decodedChars is deprecated since version 2.7 and will be removed in 3.1.', E_USER_DEPRECATED); |
There was a problem hiding this comment.
Looks suspicious: 2.7 can't get more deprecations. Only master can.
There was a problem hiding this comment.
It can't be removed in 3.1 too, only in 4.0 otherwise it will not be BC with 3.0
There was a problem hiding this comment.
Please be more clear in what I'm supposed to do. This is basically my first PR.
I am not an experienced Symfony developer, and have no clear idea what I'm supposed to do or where to find the appropriate documentation. Only telling me what I'm doing wrong, won't improve this PR :/
I'd gladly donate this change/PR to someone who actually knows what to do. I just want the route-generation to be faster, since it takes a fairly large part of the symfony 'overhead'* of our application.
* Its not really overhead, but due to its flexibility it is much more complex than simply concatenating the right parts of a url.
There was a problem hiding this comment.
@arjenm deprecations cannot be made in existing minor releases. A deprecation implies that there's a new way of doing something, also known as a feature. In order to add a feature, it has to be in the newest minor release (3.1, 3.2 etc). This PR is requested against 2.7, which doesn't accept new features anymore (same goes for any other lower than 3.1 at the moment as 3.1 is the next).
So what I think you should do, is reopen this PR against the master if you want it as a feature. This means you will have to update your message to something like "deprecated in 3.1, removed in 4.0". Currently 2.7 and 2.8 only accept bug fixes (will be explained a bit better in the docs: symfony/symfony-docs#6412).
There was a problem hiding this comment.
@arjenm you should just remove this trigger_error and add it later in another PR on master
There was a problem hiding this comment.
@nicolas-grekas while I appreciate your suggestion and it would allow early inclusion of this change. Its also not in the spirit of the intended prevention of BC-breakage. If someone would rely on the contents of the decodedChars-array, that would still break (i.e. it would be an empty array, rather than one containing several key/value pairs) and without any warning. Its already a bug waiting to happen in its current state; changing the array without changing the url matcher could result in mismatched routes...
Adding more code to test if the array was non-default, could destroy most of the performance gain.
So I'll rebase it against master, where such a change is much less unexpected. So I'll follow the suggestion of @iltar
As discussed in #18257 its actually cleaner to use a preg_replace_callback rather than a preg_match to prevent unnecessary calls to rawurlencode+strtr, but than using the rawurlencode+strtr anyway.
This patch replaces that preg_match with a preg_replace_callback.
It has similar best-case performance to the original fix. And better worst-case performance.
Besides that, its just more logical to use the regexp to find characters we do want to change to actually have that effect, rather than using another function altogether (i.e. rawurlencode+strtr).
See performance comparison of original code (i.e. not preventing using rawurlencode+strtr) to the new code (using preg_replace_callback + rawurlencode as callback) here.
http://achelois.tweakers.net/~acm/urlrawencode_or_preg_replace.php
Its in my scenario's always at least 50% faster, but can be up to 80-90%. In absolute numbers, its 'only' about 3-7ms for 1000 url's, but given the importance of the doGenerate-method, I'd say even such a smallish gain is worth it.