GH-7007: Synchronized Service alternative, backwards compatible. #7707

beberlei · 2013-04-18T08:40:35Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	GH-7007
License	MIT
Doc PR	none

Prototype for introducing a RequestContext in HttpKernel.

This PR keeps the synchronized services feature, however introduces a RequestContext object additionally, that allows to avoid using synchronized service when injecting request_context instead of request into a service. The FrameworkBundle is modified such that it does not depend on synchronized services anymore. Users however can still depend on request, activating the synchronized services feature.

Features:

Introduced REQUEST_FINSHED (name is up for grabs) event to handle global state and environment cleanup that should not be done in RESPONSE. Called in both exception or success case correctly
Changed listeners that were synchronized before to using onKernelRequestFinished and RequestContext to reset to the parent request (RouterListener + LocaleListener).
Changed FragmentHandler to use the RequestContext. Added some more tests for this class.
RequestStack is injected into the HttpKernel to handle the request finished event and push/pop the stack with requests.

Todos:

RequestContext name clashes with Routing components RequestContext. Keep or make unique?
Name for Kernel Request Finished Event could be improved.

stof · 2013-04-18T08:50:21Z

@beberlei the issue when using onKernelResponse is that your service will be broken if a previous listener stops the propagation of the response. and you still have a broken state during the end of the request (after the listener is called while calling lower priority listeners)

beberlei · 2013-04-18T08:55:56Z

@stof hm ok, that makes sense, didn't think about propagation. The state i tried to "fix" by making priority very low.

Maybe for the "stack->pop" case, we still need an event that is fired after kernel.response

stof · 2013-04-18T08:57:48Z

@beberlei I think it indeed needs an event when leaving the subrequest

fabpot · 2013-04-18T08:58:47Z

src/Symfony/Component/HttpKernel/EventListener/LocaleListener.php

    public static function getSubscribedEvents()
    {
        return array(
            // must be registered after the Router to have access to the _locale
            KernelEvents::REQUEST => array(array('onKernelRequest', 16)),
+            // should be registered very late
+            KernelEvents::RESPONSE => array(array('onKernelResponse', -255)),


You are reintroducing the exact same bug as we had before synchronized services. Using the response event is not an option here as it is not always called (think of a sub-request that throws an exception for instance).

Sorry my ignorance, It slipped me that it worked this way before. I thought this was viable to get rid of the explicit event for leaving the scope and remove the dependency on EventDispatcher in the RequestStack, but I guess I have to reintroduce this then.

I will rethink my approach.

stof · 2013-04-18T09:01:22Z

Not supporting the request stack in the normal HttpKernel will break Silex as it does not use the ContainerAware kernel but uses the listeners.

beberlei · 2013-04-18T09:02:12Z

@stof yes, but the problem is that __construct of HttpKernel is tagged with @Api so i cannot inject it there. I would really like to add it there, because it doesn't depend on the container.

fabpot · 2013-04-18T09:03:17Z

@beberlei As this is a constructor, if you add an optional argument at the end, that won't break anything.

beberlei · 2013-04-18T09:25:11Z

Updated the PR introducing a REQUEST_FINISHED event. For now its only in filterResponse in HttpKernel, maybe it needs to be added in other places as well.

I tried to avoid adding another event, but it seems for cleanup of global state + environment it makes sense to add this one.

beberlei · 2013-04-21T16:47:57Z

@fabpot Have rethought this and cannot come up with a better solution. What do you think?

fabpot · 2013-04-22T13:46:40Z

The RequestContext can also probably be used to fix another problem that we have: see #7453 (current PR on this topic #7461)

beberlei · 2013-04-24T10:21:53Z

Yes, i could extend the PR to have a create() method on the context.

beberlei · 2013-04-29T16:17:07Z

I can work on this tomorrow, is that timely enough?

beberlei · 2013-04-29T16:18:12Z

Btw I am not sure it makes sense to integrate the create() method, a dedicated factory follows the single responsibility principle much more. However I think the code in #7461 is too generic, using call_user_func_array().

…ew REQUEST_FINISHED event that is specfically for cleaning up global+environment state to avoid using RESPONSE event.

…ng during exception.

…ew classes.

beberlei · 2013-04-30T12:20:54Z

@fabpot @stof Updated the PR, adding fragment handler support, so that synchronized services are not used anymore at all in the core. Added more tests and fixed some bugs (Fragment Handler Subcontroller Functional Test was very helpful here).

I removed the WIP flag to open it up for discussion about a potential merge before 2.3 Beta 2

pborreli · 2013-04-30T12:23:55Z

src/Symfony/Component/HttpKernel/KernelEvents.php

+    /**
+     * The REQUEST_FINISHED event occurs when a response was generated for a request.
+     *
+     * This event allows you to reset the global and envioronmental state of


typo envioronmental => environmental

…#populateRoutingContext() and make it private.

igorw · 2013-05-27T22:13:12Z

src/Symfony/Component/HttpKernel/Tests/RequestStackTest.php

+        $stack->push($request);
+
+        $this->assertSame($request, $stack->getCurrentRequest());
+        $this->assertSame($request, $stack->getCurrentRequest());


Is this duplication intentional? To verify that getCurrentRequest does not mutate anything?

fabpot · 2013-08-23T16:47:53Z

src/Symfony/Component/HttpKernel/DependencyInjection/ContainerAwareHttpKernel.php

     */
-    public function __construct(EventDispatcherInterface $dispatcher, ContainerInterface $container, ControllerResolverInterface $controllerResolver)
+    public function __construct(EventDispatcherInterface $dispatcher, ContainerInterface $container, ControllerResolverInterface $controllerResolver, RequestStack $requestStack)


should accept null by default for BC.

fabpot · 2013-08-23T17:02:14Z

@beberlei If you don't mind, I'm going to rebase this PR (which has a lot of conflicts now), apply some of the changes mentioned in my comments above and resubmit a new PR for discussion before merging.

fabpot · 2013-08-31T19:26:43Z

rebased version is at #8904

This PR was merged into the master branch. Discussion ---------- Synchronized Service alternative, backwards compatible This is a rebased version #7707. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7707 | License | MIT | Doc PR | symfony/symfony-docs#2956 Todo/Questions - [x] do we deprecate the synchronize feature (and removed it in 3.0)? - [x] deal with BC for listeners - [x] rename RequestContext as we already have a class with the same name in the Routing component? - [x] move RequestStack and RequestContext to HttpFoundation? - [x] update documentation Prototype for introducing a ``RequestContext`` in HttpKernel. This PR keeps the synchronized services feature, however introduces a ``RequestContext`` object additionally, that allows to avoid using synchronized service when injecting ``request_context`` instead of ``request`` into a service. The FrameworkBundle is modified such that it does not depend on synchronized services anymore. Users however can still depend on ``request``, activating the synchronized services feature. Features: * Introduced ``REQUEST_FINSHED`` (name is up for grabs) event to handle global state and environment cleanup that should not be done in ``RESPONSE``. Called in both exception or success case correctly * Changed listeners that were synchronized before to using ``onKernelRequestFinished`` and ``RequestContext`` to reset to the parent request (RouterListener + LocaleListener). * Changed ``FragmentHandler`` to use the RequestContext. Added some more tests for this class. * ``RequestStack`` is injected into the ``HttpKernel`` to handle the request finished event and push/pop the stack with requests. Todos: * RequestContext name clashes with Routing components RequestContext. Keep or make unique? * Name for Kernel Request Finished Event could be improved. Commits ------- 1b2ef74 [Security] made sure that the exception listener is always removed from the event dispatcher at the end of the request b1a062d moved RequestStack to HttpFoundation and removed RequestContext 93e60ea [HttpKernel] modified listeners to be BC with Symfony <2.4 018b719 [HttpKernel] tweaked the code f9b10ba [HttpKernel] renamed the kernel finished event a58a8a6 [HttpKernel] changed request_stack to a private service c55f1ea added a RequestStack class

fabpot reviewed Apr 18, 2013
View reviewed changes

beberlei added 5 commits April 30, 2013 13:29

symfonyGH-7007: Synchronized Service alternative, backwards compatible.

db4d934

Move RequestStack into HttpKernel as optional dependency. Introduce n…

705f0d4

…ew REQUEST_FINISHED event that is specfically for cleaning up global+environment state to avoid using RESPONSE event.

[symfonyGH-7707] Adjusted fragmenting to not use synchronized services.

01ac8c9

[symfonyGH-7707] Fix bug with order of RequestFinished event + handli…

ec539ae

…ng during exception.

[symfonyGH-7707] Added tests for RequestStack and documentation for n…

0ef3664

…ew classes.

beberlei mentioned this pull request Apr 30, 2013

Permit HttpFoundation\Request class override #7461

Closed

pborreli reviewed Apr 30, 2013
View reviewed changes

beberlei added 2 commits April 30, 2013 14:30

[symfonyGH-7707] Fix typos

778eaea

[symfonyGH-7707] Rename RouterListener#setRequest() to RouterListener…

5d06aff

…#populateRoutingContext() and make it private.

igorw reviewed May 27, 2013
View reviewed changes

fabpot mentioned this pull request Jul 10, 2013

[HttpKernel] add parent request to subrequest #8471

Closed

fabpot reviewed Aug 23, 2013
View reviewed changes

fabpot closed this Aug 31, 2013

fabpot mentioned this pull request Sep 1, 2013

Synchronized Service alternative, backwards compatible #8904

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-7007: Synchronized Service alternative, backwards compatible. #7707

GH-7007: Synchronized Service alternative, backwards compatible. #7707

beberlei commented Apr 18, 2013

stof commented Apr 18, 2013

beberlei commented Apr 18, 2013

stof commented Apr 18, 2013

fabpot Apr 18, 2013

beberlei Apr 18, 2013

stof commented Apr 18, 2013

beberlei commented Apr 18, 2013

fabpot commented Apr 18, 2013

beberlei commented Apr 18, 2013

beberlei commented Apr 21, 2013

fabpot commented Apr 22, 2013

beberlei commented Apr 24, 2013

beberlei commented Apr 29, 2013

beberlei commented Apr 29, 2013

beberlei commented Apr 30, 2013

pborreli Apr 30, 2013

igorw May 27, 2013

fabpot Aug 23, 2013

fabpot commented Aug 23, 2013

fabpot commented Aug 31, 2013

GH-7007: Synchronized Service alternative, backwards compatible. #7707

GH-7007: Synchronized Service alternative, backwards compatible. #7707

Conversation

beberlei commented Apr 18, 2013

stof commented Apr 18, 2013

beberlei commented Apr 18, 2013

stof commented Apr 18, 2013

fabpot Apr 18, 2013

Choose a reason for hiding this comment

beberlei Apr 18, 2013

Choose a reason for hiding this comment

stof commented Apr 18, 2013

beberlei commented Apr 18, 2013

fabpot commented Apr 18, 2013

beberlei commented Apr 18, 2013

beberlei commented Apr 21, 2013

fabpot commented Apr 22, 2013

beberlei commented Apr 24, 2013

beberlei commented Apr 29, 2013

beberlei commented Apr 29, 2013

beberlei commented Apr 30, 2013

pborreli Apr 30, 2013

Choose a reason for hiding this comment

igorw May 27, 2013

Choose a reason for hiding this comment

fabpot Aug 23, 2013

Choose a reason for hiding this comment

fabpot commented Aug 23, 2013

fabpot commented Aug 31, 2013