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.
This is precisely the reason why we decided to forbid doing such fragile usage of the container (which you asked us to revert).
I can tell you that many other things are broken for such usage (try to introduce an auto-wired service in the dependency graph of VichUploaderBundle and you will then ask us to move the auto-wiring logic out of compiler passes too)

Fact is, we have a BC policy that promises: upgrade to next minor wont break your code.
VichUploaderBundle was not working "by luck". It was working "by fact". And this is perfectly fine. Bootstrapping is a complex issue. It doesn't happen all of a sudden but step by step.

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!
But in this case, we're not making ones code more robust. Instead, we're moving the problem elsewhere, in user's hand, without providing any better alternative than saying "we don't care, do as you want". This is bad and this deprecation should be reverted for that reason. We still can think of other ways to make the bootstrapping process more robust, but I call that deprecation a dead end.

Theonly 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 this is impossible, as you cannot make the ContainerBuilder aware of all the logic running in compiler passes (as they are extension point) and able to detect whether the corresponding compiler pass ran already or no (to avoid redoing the same resolution a second time, which could break things).

Andany 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.
This is similar to cases where Drupal complained in the past that a symfony update broke their code in a place where they were extending an internal class. We have not reverted our changes to maintain BC there. We helped Drupal stop relying on the unsupported usage of the framework.

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.

I'm mostly 👎 on this, and probably not in 2.7 anyway.

@nicolas-grekas this also fixes theschmittjoh/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.
To me, we have a responsibility to add this, let's say feature, in 3.2, so that we respect our BC promise.
I feel bad with saying to two unrelated projects now, more later: we broke your code but you're the bad guy because whatever. Note that I don't run these projects so I do care only because I feel responsible for the BC promise as a core team member.
If you feel confident with another pov, I'll let you justify it (who ever is that "you") :-)

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 theirAnnotationConfigurationPass is registered to be run before any optimisation passes (seehttps://github.com/schmittjoh/JMSDiExtraBundle/blob/master/JMSDiExtraBundle.php#L39-L41).This means that it is trying to get services from the container before things like service decoration, autowiring and so has happened which makes it even more likely that there will be similar issues in the future. Do we want to move all these things to the container builder?

@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.
Yet, that's not enough, because cache pools need a logger. But since the logger there is optional, I made it possible for the container builder to build partial services when this is compatible with the service definition (on-invalid=ignore).
To do so, we need a newUnresolvedServiceDefinitionException class.
Confirmed to fix the issue for VichUploaderBundle.

If that looks like a better approach to you, I just need to deal with tests then.
So before doing so, WDYT?

Note also that another way is possible: filling all DefinitionDecorator objects with actual values from their parentswithout replacing them andbefore 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

@stof seehttps://github.com/symfony/symfony/pull/20537/files#diff-a10c9afd8feaccd1ff66ffd54e2835bcR846

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

it does not solve things for other errors happening there because of other kind of no-fully-configured services

Sure! We can make that for 3.3, here I'm just trying to figure out the viable minimum for 3.2

PR ready, tests green. RFR

ping@stof@fabpot@xabbuh & @symfony/deciders

Doesn't work because of tags, which are lost when parents are resolved. I give up sorry.