[DependencyInjection] IntrospectableContainerInterface::initialized() #3557
Conversation
|
Do you have any other examples in mind where it would be useful? |
|
@fabpot 2 exemples:
|
|
However, I don't like the name of the method |
|
sounds very useful |
|
@fabpot another example:
@stof Would more or less verbose be better? Like, |
|
@evillemez My issue is the word |
|
one thing we should also keep in mind here is the scope of the service. BTW: there are also a couple of CS violations in your PR |
|
@stof: I agree that we should think of a better name: @evillemez: After choosing the name, can you work on actually using the new method for some of the use cases mentioned in this PR? |
|
what i meant with "scope" was if we are only talking about services instantiated in the current scope? but i guess there is no way to handle anything else anyway. as for name .. i think |
|
The current implementation only works for container-scoped services. It does not make sense for prototyped-scope services anyway (as the container does not keep them) but supporting scoped services will be tricky IMO |
|
hasBeen(Used|Called|Initialized), |
|
The next day or two I'm only around for an hour or so here and there, I may be a little slow to respond. @stof @lsmith77 I agree with either @lsmith77 Besides the opening/closing brace placements, was there anything else? @fabpot I would happily implement it in the Swiftmail bundle - as of now that's the only area where I know absolutely it would be useful. The Session example I mentioned was an issue I ran into working on another app in another framework, and I'm not familiar yet with the internals of the Monolog/Doctrine Bundles. But if people could point me in the right direction I'd be willing to check it out. I'm still relatively new to some of the Symfony2 internals, so if there are obvious things I'm missing, please don't hesitate to point them out. :) |
|
@lsmith77 Oh... I think there were some tab issues I didn't see in my editor, I'll fix those too. |
|
The places where it should be used for Doctrine and Monolog are in separate repos anyway so it cannot be part of this PR |
|
Any thoughts on How should I proceed, close this request and submit another with the changed name and fixed CS violations? |
|
|
|
I was about to squash my commits to update this, but it just occurred to me that I hadn't considered the interface. Does anyone feel this method |
|
We cannot break BC for |
|
Is it a BC break if we add a method? i thought only changing the already written methods would be a BC break. |
|
@henrikbjorn It will raise a fatal error if the method isn't implemented in existing class. |
|
we could however add a new interface that extends from the previous one. |
|
Are the BC breaks we are worried about for compatibility just within the Symfony repo - or in general in case others have implemented the interface elsewhere? As far as Symfony is concerned, the only class I can find that implements If it's an issue of principle, in case others may have implemented the Is this more or less acceptable than updating the interface? |
|
Hadn't properly squashed commits, just fixed. |
|
I'm done with this PR, unless there is a consensus that I should also implement Anything else I need to do? |
|
@evillemez the common pattern for BC is to add a new interface, say IntrospectableContainerInterface or something, that extends ContainerInterface, then you can type-hint that without restricting use of competing implementations of the Container class. |
|
@evillemez Please update this PR. It conflicts with master because of the move of tests. And the new interface suggested by @Seldaek is a good idea IMO |
|
@stof I may not be able to get to this until the end of the week, but I'll do it as soon as I can. I'll rebase against the current master, and add/implement the interface. Is Are there other features that we can think of that would be useful for this new interface? I don't want to start a precedent of adding a new interface for every new method that comes up... I understand it makes sense for not breaking backwards compatibility for something previously marked as stable, but still, it's yet another file that will likely be included on every request. |
|
@evillemez classes used on every requests can be added to some cached bootstrap files (which are loaded during the |
|
Ok, rebased against current master and implemented the interface. I didn't mark it as Sorry for the delay. My wife and I are in the process of buying a house, and some unexpected things have come up. |
| * | ||
| * @return Boolean true if service has already been initialized, false otherwise | ||
| */ | ||
| public function initialized($id) |
There was a problem hiding this comment.
Why not use {@inheritdoc} here?
And the interface PHPDoc does not mention the return.
There was a problem hiding this comment.
@inheritdoc specifically for this method, or for IntrospectableContainerInterface? I didn't use @inheritdoc here because I didn't see it done anywhere else for this class. Should I? Good catch on the interface docs, fixing.
|
Can you rebase before I merge? Thanks. |
|
Sure thing. |
|
You need to rebase, not merge. |
|
Yeah, I just made a mess of this. Figuring out how to properly rebase, sorry for the delay. |
|
@evillemez (rebasing)[http://symfony.com/doc/current/contributing/code/patches.html#id1] and then you need to force push |
|
Thanks, I think the issue I'm having is that I submitted this PR from my master branch, I have another branch where I think I just worked this out properly... but it's a different branch. |
|
Yeah... that wasn't it. Would this work better if I just closed this PR, and submitted a new PR from the other branch? |
|
then do git fetch upstream and git rebase upstream/master |
|
Ok... I -think- that did it. Thanks so much for the patience, clearly I haven't had to use rebase before. :) |
…tainerInterface::initialized()
|
Just squashed commit to fix a syntax issue in the test |
Commits ------- b7b26af [DependencyInjection] Added, implemented and tested IntrospectableContainerInterface::initialized() Discussion ---------- [DependencyInjection] IntrospectableContainerInterface::initialized() Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Added, implemented and tested `IntrospectableContainerInterface::initialized()`, which allows checking for whether or not a service id has actually been loaded, without forcing it to load. This could be implemented in several places to prevent loading a service when it's only needed if it has been previously loaded - for example the SwiftmailBundle's event listener for the `kernel.terminate` event, which currently forces Swiftmail to load on every request to check for messages to send, even if it was not previously loaded. --------------------------------------------------------------------------- by fabpot at 2012-03-11T09:10:32Z Do you have any other examples in mind where it would be useful? --------------------------------------------------------------------------- by stof at 2012-03-11T12:39:22Z @fabpot 2 exemples: - the Swiftmailer listener to avoid loading the initialization code of Swiftmailer in each kernel.terminate event just to check that there is no pending message in the spool (if the spool has never been retrieved, it cannot have messages) (this is the use case we discussed on IRC last night) - closing Doctrine connections and Monolog handlers in the shutdown method without creating them if they were not used --------------------------------------------------------------------------- by stof at 2012-03-11T12:39:53Z However, I don't like the name of the method --------------------------------------------------------------------------- by lsmith77 at 2012-03-11T13:26:34Z sounds very useful --------------------------------------------------------------------------- by evillemez at 2012-03-11T17:00:39Z @fabpot another example: * Forcing a session to write early, if it was previously loaded - but not having to load the session to check, thus potentially forcing a database connection (if that's how the session is being handled) when it's not needed. @stof Would more or less verbose be better? Like, `isServiceLoaded()` or just `loaded()`.... or a better word than "loaded" ? :) --------------------------------------------------------------------------- by stof at 2012-03-11T17:03:11Z @evillemez My issue is the word ``loaded``. I don't think it is clear about what it means here --------------------------------------------------------------------------- by lsmith77 at 2012-03-11T17:04:24Z one thing we should also keep in mind here is the scope of the service. BTW: there are also a couple of CS violations in your PR --------------------------------------------------------------------------- by fabpot at 2012-03-11T17:07:43Z @stof: I agree that we should think of a better name: `initialized()` or `exists()` (to differentiate from `has()`)? @evillemez: After choosing the name, can you work on actually using the new method for some of the use cases mentioned in this PR? --------------------------------------------------------------------------- by lsmith77 at 2012-03-11T17:20:12Z what i meant with "scope" was if we are only talking about services instantiated in the current scope? but i guess there is no way to handle anything else anyway. as for name .. i think ``instantiated()`` is more fitting than ``initialized()``. ``exists()`` could be confused with ``has()`` imho --------------------------------------------------------------------------- by stof at 2012-03-11T17:26:06Z The current implementation only works for container-scoped services. It does not make sense for prototyped-scope services anyway (as the container does not keep them) but supporting scoped services will be tricky IMO --------------------------------------------------------------------------- by mvrhov at 2012-03-11T17:34:21Z hasBeen(Used|Called|Initialized), --------------------------------------------------------------------------- by evillemez at 2012-03-11T17:54:43Z The next day or two I'm only around for an hour or so here and there, I may be a little slow to respond. @stof @lsmith77 I agree with either `instantiated()` or `initialized()`, I also think `exists()` might be easily confused with `has()` @lsmith77 Besides the opening/closing brace placements, was there anything else? @fabpot I would happily implement it in the Swiftmail bundle - as of now that's the only area where I know absolutely it would be useful. The Session example I mentioned was an issue I ran into working on another app in another framework, and I'm not familiar yet with the internals of the Monolog/Doctrine Bundles. But if people could point me in the right direction I'd be willing to check it out. I'm still relatively new to some of the Symfony2 internals, so if there are obvious things I'm missing, please don't hesitate to point them out. :) --------------------------------------------------------------------------- by evillemez at 2012-03-11T18:00:29Z @lsmith77 Oh... I think there were some tab issues I didn't see in my editor, I'll fix those too. --------------------------------------------------------------------------- by stof at 2012-03-11T18:13:03Z The places where it should be used for Doctrine and Monolog are in separate repos anyway so it cannot be part of this PR --------------------------------------------------------------------------- by evillemez at 2012-03-12T03:38:50Z Any thoughts on `instantiated` vs `initialized`? I'm leaning towards `initialized`. How should I proceed, close this request and submit another with the changed name and fixed CS violations? --------------------------------------------------------------------------- by fabpot at 2012-03-12T07:41:11Z `initialized()` looks fine to me. Make your changes, squash your commits and then force the push to your branch (the PR will be updated automatically). --------------------------------------------------------------------------- by evillemez at 2012-03-12T20:49:17Z I was about to squash my commits to update this, but it just occurred to me that I hadn't considered the interface. Does anyone feel this method `initialized()` should be defined in ContainerInterface as well? It seems like it's generic enough that it would make sense. But I'm not immediately aware if this would cause BC breaks. --------------------------------------------------------------------------- by fabpot at 2012-03-13T11:34:33Z We cannot break BC for `ContainerInterface` as this is marked with the `@api` tag. --------------------------------------------------------------------------- by henrikbjorn at 2012-03-13T12:34:42Z Is it a BC break if we add a method? i thought only changing the already written methods would be a BC break. --------------------------------------------------------------------------- by ooflorent at 2012-03-13T12:39:44Z @henrikbjorn It will raise a fatal error if the method isn't implemented in existing class. --------------------------------------------------------------------------- by lsmith77 at 2012-03-13T13:06:26Z we could however add a new interface that extends from the previous one. --------------------------------------------------------------------------- by evillemez at 2012-03-13T15:40:39Z Are the BC breaks we are worried about for compatibility just within the Symfony repo - or in general in case others have implemented the interface elsewhere? As far as Symfony is concerned, the only class I can find that implements `ContainerInterface` is `Container`, so adding the method shouldn't be an issue. If it's an issue of principle, in case others may have implemented the `ContainerInterface`, then... yeah, it's a break, and maybe should be left out. I suppose this would mean that other places in code that type hint for `ContainerInterface` would have to change the type hint to `Container` if they want to implement the `initialized()` method. Is this more or less acceptable than updating the interface? --------------------------------------------------------------------------- by evillemez at 2012-03-14T19:17:27Z Hadn't properly squashed commits, just fixed. --------------------------------------------------------------------------- by evillemez at 2012-03-15T14:06:38Z I'm done with this PR, unless there is a consensus that I should also implement `Container::initialized()` in `ContainerInterface`. Anything else I need to do? --------------------------------------------------------------------------- by Seldaek at 2012-03-15T15:41:44Z @evillemez the common pattern for BC is to add a new interface, say IntrospectableContainerInterface or something, that extends ContainerInterface, then you can type-hint that without restricting use of competing implementations of the Container class. --------------------------------------------------------------------------- by stof at 2012-04-03T22:30:51Z @evillemez Please update this PR. It conflicts with master because of the move of tests. And the new interface suggested by @Seldaek is a good idea IMO --------------------------------------------------------------------------- by evillemez at 2012-04-04T14:57:29Z @stof I may not be able to get to this until the end of the week, but I'll do it as soon as I can. I'll rebase against the current master, and add/implement the interface. Is `IntrospectableContainerInterface ` as suggested by @Seldaek ok with everyone? Are there other features that we can think of that would be useful for this new interface? I don't want to start a precedent of adding a new interface for every new method that comes up... I understand it makes sense for not breaking backwards compatibility for something previously marked as stable, but still, it's yet another file that will likely be included on every request. --------------------------------------------------------------------------- by stof at 2012-04-04T15:35:15Z @evillemez classes used on every requests can be added to some cached bootstrap files (which are loaded during the ``$kernel->loadClassCache()`` call in your front controller) to avoid including many files through the autoloader. And for even better performances in prod, the solution is to use APC with ``apc.stat = 0``, as advocated by @lsmith77 --------------------------------------------------------------------------- by evillemez at 2012-04-15T19:00:07Z Ok, rebased against current master and implemented the interface. I didn't mark it as `@api` because I think we may want to consider if there's any other functionality that could be implemented there before declaring it stable. Sorry for the delay. My wife and I are in the process of buying a house, and some unexpected things have come up. --------------------------------------------------------------------------- by fabpot at 2012-04-18T10:59:01Z Can you rebase before I merge? Thanks.
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Added, implemented and tested
IntrospectableContainerInterface::initialized(), which allows checking for whether or not a service id has actually been loaded, without forcing it to load. This could be implemented in several places to prevent loading a service when it's only needed if it has been previously loaded - for example the SwiftmailBundle's event listener for thekernel.terminateevent, which currently forces Swiftmail to load on every request to check for messages to send, even if it was not previously loaded.