[FrameworkBundle][DI] Resolve cache pool adapters in CachePoolPass #20537
Conversation
|
This looks wrong to me. The issue is that VichUploaderbundle expects to have a service and all its dependencies working during container building, and this would forbid using compiler passes for any logic then, as it would mean that you would have to make the container working even if compiler passes are not running. We have lots of cases where this assumption does not hold true, not only definition inheritance. VichUploaderBundle was just lucky that its dependency graph was not relying on such a feature in the past. |
|
Fact is, we have a BC policy that promises: upgrade to next minor wont break your code. You say this PR is wrong. I say it's legitimate. |
|
@nicolas-grekas we are advocating since years that getting a service instance inside compiler pass is not a supported use case and that you are on your own when doing it, as there is no way to be sure it works. The only way to maintain BC if you want to avoid breaking existing case that worked by luck before is to strictly forbid changing an existing service definition to start using a feature resolved by a compiler pass (and so forbidding any existing service to start depending on cache pools, which use service inheritance, if they were not relying on definition inheritance before, but also forbidding them to add a dependency to an autowired service if they were not relying on auto-wiring before, etc...) |
|
I'm all for making things more robust yes! |
|
The only way to be sure that the ContainerBuilder can instantiate a service properly before compiler passes are run is to make the ContainerBuilder perform all the logic to configure the service before it gets initialized. And any change to a service definition could make all dependant services move from working to not working in a compiler pass, or the opposite (which depends of which compiler pass it is, as the service itself will move from not working to working at some point, and the graph of services depending on it may change over time too as compiler passes are reconfiguring the container). |
|
I agree with these statements! Yet they don't invalidate the two PRs I submitted :) |
|
Well, your PR tries to make part of the impossible thing, to have 1 case working. but if we start doing it, we will get complains about all other cases (autowiring and service decorations are coming to my mind, and service decoration would be hard to detect in the ContainerBuilder btw). My issue with your changes is that you try to turn this unsupported use case into a supported one, which would then mean we should support it properly (and we cannot). The right fix would be for VichUploaderBundle to rewrite their feature to stop relying on unsupported usage of Symfony. |
|
Btw, with a small change, VichUploaderBundle could be broken only for propel users instead of for everyone, as this is inside the compiler pass registering the propel integration. |
|
|
||
| /** | ||
| * Creates a new Definition by merging the current decorator with the given parent definition. | ||
| * |
There was a problem hiding this comment.
We don't add @param when they don't add any value to the method signature, which is the case here.
| @@ -840,6 +840,24 @@ public function findDefinition($id) | |||
| */ | |||
| public function createService(Definition $definition, $id, $tryProxy = true) | |||
There was a problem hiding this comment.
So.. as this is public API should we deprecate calling it as well instead of get() before compiling? ^^ Opposed to #20533 of course ;-)
Including resolveServices that is, as right now a reference will trigger the deprecation (ref), whereas a definition doesnt.
Imo. we should move forward with this, but i do agree with you in terms of providing an alternative solution. Lets focus on that?
There was a problem hiding this comment.
createService should be marked as internal actually. It was public by necessity to support PHP 5.3, as it is called inside the closure of the proxy instantiation for lazy services
There was a problem hiding this comment.
actually, it is already internal (and private in Symfony 3)
|
I'm mostly 👎 on this, and probably not in 2.7 anyway. |
df261c2 to
22d2009
Compare
22d2009 to
1d4d6e7
Compare
|
@nicolas-grekas this also fixes the schmittjoh/JMSDiExtraBundle#262 as well it seems for 3.2.0-beta1 |
|
PR rebased on 3.2. I'm really fine with merging this only there, because we are using definition decorators more extensively since 3.2, so it matches the need for it. Yet, we have now two projects that get a fatal error at boot time. I bet we'll get way more when getting out of the beta. |
|
I am afraid that if we started this way, we would finally end up with moving more and more stuff from compiler passes to the container builder just because there will be other similar edge cases related to other compiler pass features. |
|
By the way, the case for JMSDiExtraBundle becomes even worse as their |
|
@xabbuh no we don't (service decoration would not be able to be detected when asking to instantiate a service btw, without actually running the compiler pass, as this is triggered by other definitions). I'm OK doing it for DefinitionDecorator only, to account for the fact that we rely on them more extensively and we reverted #20533, but we should start the process of helping bundle maintainers to stop using services before they are built, so that we can reintroduce safeguards in the future (and avoid the need to move more logic to the ContainerBuilder) |
|
Here is another way to fix the fatal error on those bundle. I kept the first commit the same, and added the new idea on top as a second commit: Instead of allowing the container builder to resolve definition decorators, I made the CachePoolPass resolve them for its cache pools. Makes the fix more local. If that looks like a better approach to you, I just need to deal with tests then. |
|
Note also that another way is possible: filling all DefinitionDecorator objects with actual values from their parents without replacing them and before any other compiler pass. That would make the container builder able to resolve them without relying on ResolveDefinitionTemplatesPass, while preserving the definition graph for user land passes. Let me know if you think this should be the way to go instead. |
|
@nicolas-grekas this could break things if the parent definition is altered by a compiler pass (or make the code a lot more complex to avoid such break). And regarding UnresolvedServiceDefinitionException, the ContainerBuilder has no way to know that some config of the service has not been done yet (it does not know what is the expected state of the service, and if an error occurs, it cannot know whether an upcoming config change would fix it). So I don't see where it would be triggered |
| @@ -1120,6 +1131,19 @@ private function callMethod($service, $call) | |||
| call_user_func_array(array($service, $call[0]), $this->resolveServices($this->getParameterBag()->unescapeValue($this->getParameterBag()->resolveValue($call[1])))); | |||
| } | |||
|
|
|||
| private function resolveDefinitionDecorator(DefinitionDecorator $definition, $id) | |||
|
well, if this is only about unresolved DefinitionDecorator, indeed. But then it does not solve things for other errors happening there because of other kind of no-fully-configured services |
c32c6c6 to
2c86d33
Compare
Sure! We can make that for 3.3, here I'm just trying to figure out the viable minimum for 3.2 |
df51156 to
1db236e
Compare
|
PR ready, tests green. RFR |
1db236e to
b35f940
Compare
b35f940 to
f2f86f1
Compare
|
Doesn't work because of tags, which are lost when parents are resolved. I give up sorry. |
…"cache.annotations" (nicolas-grekas) This PR was merged into the 3.2 branch. Discussion ---------- [FrameworkBundle] Don't rely on any parent definition for "cache.annotations" | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Instead of a generic approach that failed in #20537, let's focus on the `cache.annotations` service, which is the one that needs special care because it can be required while the container is being built. See e.g.: - #20234 - http://stackoverflow.com/questions/39625863/vichuploadbundle-inb-symfony-3-cant-load-cache-annotations-service/40626277 - schmittjoh/JMSDiExtraBundle#262 When the service is required at build time, we can't provide it a logger, because no logger service is ready at that time. Still, that doesn't prevent the service from working. The late `CachePoolClearerPass` wires the logger for later instantiations so that `cache.annotations` has a properly configured logger *for the next requests*. Commits ------- f62b820 [FrameworkBundle] Dont rely on any parent definition for "cache.annotations"
This issue has been reported here:
http://stackoverflow.com/questions/39625863/vichuploadbundle-inb-symfony-3-cant-load-cache-annotations-service/40626277
It's been not noticed until 3.1/3.2, where cache pool definitions make extensive use of parent definitions.
Even if the change set looks big, it's just moving some common logic into DefinitionDecorator, so that we can reuse it in ContainerBuilder.
The current outcome of the bug is a fatal error when bootstrapping the container. The reproducer is easy: just enable the VichUploaderBundle with a 3.2 enabled standard edition.