Exiting a subrequest cannot restore request context #6756
Comments
|
As per the discussion on IRC, I think this links in with the ability to generate Lazyloading proxy objects for services (#5102, #6140) Where I'm at I like @Crell's 4th option, where the container will call setRequest($request | null) or some other such function when entering or exiting the request scope, allowing the Matcher and Generator to have a dependency on request scoped services but be defined at the container scope. Since it avoids the need of coupling these services to the container or its implementation (though the proxy solution achieves the same) it feels the neatest. |
|
probably worth reading #5300 which is somewhat related. |
|
I have a working prototype for option 4! No need to say that I've very excited as it's going to fix a bunch of issues and ease working with services that need the request as a dependency. The implementation is very different from what is suggested here, but the end result is the same. I will find some time later this week to finish the code and publish it to gather feedback. I know, I like teasing ;) |
This PR was merged into the master branch. Discussion ---------- [2.3] [WIP] Synchronized services... | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #5300, #6756 | License | MIT | Doc PR | symfony/symfony-docs#2343 Todo: - [x] update documentation - [x] find a better name than contagious (synchronized)? refs #6932, refs #5012 This PR is a proof of concept that tries to find a solution for some problems we have with scopes and services depending on scoped services (mostly the request service in Symfony). Basically, whenever you want to inject the Request into a service, you have two possibilities: * put your own service into the request scope (a new service will be created whenever a sub-request is run, and the service is not available outside the request scope); * set the request service reference as non-strict (your service is always available but the request you have depends on when the service is created the first time). This PR addresses this issue by allowing to use the second option but you service still always has the right Request service (see below for a longer explanation on how it works). There is another issue that this PR fixes: edge cases and weird behaviors. There are several bug reports about some weird behaviors, and most of the time, this is related to the sub-requests. That's because the Request is injected into several Symfony objects without being updated correctly when leaving the request scope. Let me explain that: when a listener for instance needs the Request object, it can listen to the `kernel.request` event and store the request somewhere. So, whenever you enter a sub-request, the listener will get the new one. But when the sub-request ends, the listener has no way to know that it needs to reset the request to the master one. In practice, that's not really an issue, but let me show you an example of this issue in practice: * You have a controller that is called with the English locale; * The controller (probably via a template) renders a sub-request that uses the French locale; * After the rendering, and from the controller, you try to generate a URL. Which locale the router will use? Yes, the French locale, which is wrong. To fix these issues, this PR introduces a new notion in the DIC: synchronized services. When a service is marked as synchronized, all method calls involving this service will be called each time this service is set. When in a scope, methods are also called to restore the previous version of the service when the scope leaves. If you have a look at the router or the locale listener, you will see that there is now a `setRequest` method that will called whenever the request service changes (because the `Container::set()` method is called or because the service is changed by a scope change). Commits ------- 17269e1 [DependencyInjection] fixed management of scoped services with an invalid behavior set to null bb83b3e [HttpKernel] added a safeguard for when a fragment is rendered outside the context of a master request 5d7b835 [FrameworkBundle] added some functional tests ff9d688 fixed Request management for FragmentHandler 1b98ad3 fixed Request management for LocaleListener a7b2b7e fixed Request management for RequestListener 0892135 [HttpKernel] ensured that the Request is null when outside of the Request scope 2ffcfb9 [FrameworkBundle] made the Request service synchronized ec1e7ca [DependencyInjection] added a way to automatically update scoped services
This is a bit involved, so I will try to explain it as well as I can.
We're working on stitching the CMF Routing component into Drupal: http://drupal.org/node/1874500
That involves a DynamicRouter service (part of CMF Routing), which is RequestContextAware. The Matcher and Generator in use are also RequestContextAware. OK, so put the request context in Container and inject it, no problem.
Problem: That makes the Router object request-scope sensitive, so subrequests will always trigger a new router, matcher, and generator. That works, but is not performant as @fabpot noted in the issue above.
Solution, per fabpot: Inject the context to the router via a listener before matching, rather than via the constructor. Then it gets updated prior to every match(), which is fine, and we only need one instance. Great. We'd have to override some constructors (UrlMatcher and DynamicRouter, specifically), but doable. The current RouterListener does this, albeit in a somewhat ugly way. (It's modifying a quasi-global via match(), so that it affects the generator. Ick.)
Problem: That doesn't do anything for exiting a subrequest. When exiting a subrequest, the matcher no longer matters but the generator may still be used. (No may in Drupal's case; will still be used.) The generator, however, is still referencing the context object that was last set on it, which is... the context object from the subrequest, because that's when setContext() was last called (and/or the quasi-global was last updated by the listener). That means the generator is operating on the wrong request context, which may or may not be sufficiently different to cause problems.
I talked with @merk in IRC, and we confirmed this seems to be a problem. The possible solutions we came up with include:
The fourth option seems like the most powerful, but also likely the most complex. (Naturally.)
For the moment, I am going to go with the first option in Drupal so that we can move forward. However, given that we are likely to fire a not-small number of subrequests I think we will need a solution here.
Thoughts?
The text was updated successfully, but these errors were encountered: