Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scope specific DIC #5300

Closed
Josiah opened this issue Aug 20, 2012 · 12 comments
Closed

Scope specific DIC #5300

Josiah opened this issue Aug 20, 2012 · 12 comments

Comments

@Josiah
Copy link
Contributor

Josiah commented Aug 20, 2012

Almost every time that I have had a service which is dependant on the request you need to inject the entire container. The odd part is that these services are usually very lightweight and simple functionality.

For example, lets say that I want to create a KnpMenu voter which looks at the current URI of the request and then votes for the current menu item. Because the knp_menu.matcher service isn't in the request scope, I will create a scope widening injection error if I tag my request service with knp_menu.voter.

Ideally I should be able to tag a service which is dependant on request with knp_menu.voter and have it only be injected into knp_menu.matcher when the DIC is in the request scope.

I am happy to work on a PR to implement this but it would be good to know if there is anything that would stop this kind of functionality from being accepted first.

@Josiah
Copy link
Contributor Author

Josiah commented Aug 20, 2012

I gained a better understanding of posts from the How to work with Scopes cookbook entry so I realise that the issue is a bit more complex than I first understood.

It would still be great to be able to manage references to narrower scopes without injecting the entire container. I propose the following:

<service id="example" class="%example.class%">
    <argument>Foo</argument>
    <argument>Bar</argument>
    <!-- Called on entry into the 'request' scope -->
    <call method="setRequest" scope="request">
        <argument id="request" type="service" />
    </call>

    <!-- Called on initial setup and exit from the 'request' scope -->
    <call method="setRequest"><argument /></call>
</service>

This would allow controlled support for narrower scopes without needing to inject the entire container. The service would be responsible for managing its own behaviour outside the request scope.

@bamarni
Copy link
Contributor

bamarni commented Aug 20, 2012

From what I understand about the container, to implement your suggestion, it would need to iterate over the already created services of a given scope and call a method instead of just removing them like it is currently, I don't think it's possible because at this step service are just some runtime classes agnostic about any definition instructions.

IMO for your situation passing the container is a good solution, because you can't really match with sub requests as uri are internal stuff.
BTW there is a pending PR for an uri voter on knp menu bundle, wich I hope will be merged soon :) :
KnpLabs/KnpMenuBundle#133

@schmittjoh
Copy link
Contributor

@Josiah, your second solution sounds good.

If you work on an implementation, please keep in mind that performance is a huge concern when modifying the runtime parts of the DIC.

@fabpot
Copy link
Member

fabpot commented Aug 29, 2012

Anyone working on an implementation? It sounds like a good addition for Symfony 2.2.

@stof
Copy link
Member

stof commented Aug 29, 2012

this looks indeed useful

@Josiah
Copy link
Contributor Author

Josiah commented Aug 29, 2012

I started looking into it but I got sidetracked.

It would be fairly easy to implement should a change in scope trigger transitions for all services with transitions in them, however this could slow down scope changes drastically.

I'm trying to think of a way of making the scope transitions lazy so that they are only effected when a service is referenced (in the get method of the container). However I probably won't have time to get stuck back into this PR for a few weeks.

@stof
Copy link
Member

stof commented Aug 29, 2012

@Josiah It should not be done in the get method, as this would not work for services injected inside anoher service (which is the main use case as services accessed only at runtime can generally be moved to the request scope in such case)

@Josiah
Copy link
Contributor Author

Josiah commented Aug 29, 2012

That's a good point. I might see about implementing something that just manages the transitions between scopes for now, and then look at ways to improve performance once its done.

Ideally any performance hit won't happen unless transitions have been defined in the container anyway.

@fabpot
Copy link
Member

fabpot commented Jan 16, 2013

Adding this with the current architecture is probably not possible without adding something "new". Managing transitions could be done in the enterScope and leaveScope methods, but the problem is that when you enter the scope, some of the services might not be available yet. Here is a snippet of code from the kernel:

$this->container->enterScope('request');
$this->container->set('request', $request, 'request');

@merk
Copy link
Contributor

merk commented Jan 16, 2013

Could something like work?:

$this->container->openScope('request');
$this->container->set('request', $request, 'request');
$this->container->enterScope('request');

@fabpot
Copy link
Member

fabpot commented Jan 16, 2013

Ok, I've found a workaround which should be doable without changing anything. I have a working prototype when not using the dumper (which should be easy to do after everything work for the main ContainerBuilder class). I'm going to create a pull request later on.

@Crell
Copy link
Contributor

Crell commented Jan 25, 2013

fabpot, any sign of the prototype? We'd love to see what you're cooking up. :-)

fabpot added a commit that referenced this issue Mar 23, 2013
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
@fabpot fabpot closed this as completed Mar 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants