Move query string hack for request context to listener #662
Conversation
Currently the UrlMatcher depends on the request context which leads to all kinds of funky problems. Including failing tests in the current test suite. The solution is to have the request context no longer directly depend on the request, but instead populate the required information from a request listener.
|
Refs #661. |
|
|
||
| $request = $event->getRequest(); | ||
| if ($request->server->get('QUERY_STRING') !== '') { | ||
| $this->app['request_context']->setParameter('QUERY_STRING', $request->server->get('QUERY_STRING')); |
There was a problem hiding this comment.
Shouldn't you inject only the RequestContext ?
There was a problem hiding this comment.
Wouldn't that force instantiation, when it might not be necessary?
There was a problem hiding this comment.
Yes, The app is injected for laziness. I did not want to create a LazyRequestContext, although if deemed necessary I can adjust the patch to do so.
There was a problem hiding this comment.
@davedevelopment It would indeed force the instantiation of the request context when instantiating the dispatcher... but this is already done by the RouterListener a few lines above:
$dispatcher->addSubscriber(new RouterListener($urlMatcher, $app['request_context'], $app['logger']));There was a problem hiding this comment.
@igorw no need to create a lazy one as it is already instantiated anyway
There was a problem hiding this comment.
True. I will adjust my patch to inject the context directly.
No need for laziness, since it is created early (with the dispatcher) either way.
|
👍 As @igorw mentioned, I think this could possibly have been treat as a bug in Symfony and fixed in the other branches, but I'd rather see this PR merged and reversed later if that's the case. |
|
I've implemented the BC layer in a slightly different way, which I think is better: see #663 |
Currently the UrlMatcher depends on the request context which leads to all
kinds of funky problems. Including failing tests in the current test suite.
The solution is to have the request context no longer directly depend on
the request, but instead populate the required information from a request
listener.
Better names for the listener are welcome.
PS: This solution feels kind of off to me. Shouldn't this case be handled by symfony/routing's
RequestContext::fromRequest()directly? And if not, why?