👍 for the revert.

Thank you for this fix :) 👍

Getting a service instance in a compiler pass is very fragile, as you have no guarantee that the service definition is in a working state at this point (which implies having all dependencies in a working state too).
And it leads to lots of bug reports

-1 for this, for the reason I gave in#20537 (comment)

Agree with@stof you shouldnt (and imo. mustnt) get servives before/during compilation. If you want some, compile a different container first which you can use for building the 2nd container.

The "should not do this" or "is fragile" arguments are fine for a discussion. Yet, I invite you to open real code and make it work with this constraint you are saying is good. In my experience, you'll get worse code in the end.
Challenge: please open a PR on VichUploaderBundle to fix the current deprecation on 3.2, without making the bundle ugly of course.

Again: bootstrapping is a complex issue, done step by step, and fragile by nature. Once in the bootstrapped comfort zone, things are easy. Before, "pureness" theory does not apply.

Compile a new container with themetadata_reader service (or perhaps just create the service at this point) => inject it into the pass?

bootstrapping is a complex issue, done step by step

Exactly;step by step.

@nicolas-grekas the issue is that a service can depend on lots of other services. and as soon as any of them comes from a different bundle, you haveno way to know that it will be usable at the time your compiler pas runs (and even no way to be sure that this will stay true in the future)

👍 for reverting

Anyway, if we allow cheating; what about an explicit version?ContainerBuilder::getAtOwnRisk() that is. Im very -1 about removing this integrity check fromget().

Im not even sure we can do both actually :))

I updated this PR so that the revert is only about this very deprecation, and not about enhancements that were made to tests at the same time.

Thank you@nicolas-grekas.

I am not convinced that reverting here will be the right solution on the long term. It "fixes" the issues VichUploaderBundle and JMSDiExtraBundle have right now, but in fact we just say that they should do what they want in their compiler passes and the end user has to deal with all the issues that arise.

The main issue is that when they are instanciating some of the services the end user may be unlucky that their own services will be created due to some dependencies too. This may lead so several (not easy to solve) issues I have seen in some projects which you will then have to solve with ugly code in the application. For example, services having direct or indirect dependencies on the kernel will simply fail because the service is synthetic and simply not available at this stage (you can only solve this by injecting the full container and then do service location later on). Another example is when your own compiler passes are executed later on and then do not behave as you thought just because services for your definitions (or other definitions) have already been created.

In my opinion we should at least reconsider this decision for 3.3 and rather help bundle maintainers to update their code. It is from my point of view better to have some not so good looking code in a few third-party bundles than causing WTF moments to the end user or forcing them to organise their services in a certain way (and we can also easier help bundle maintainers on GitHub than every single application developer out there).

I think that you are focusing too much in the deprecation/no-deprecation discussion. If VichUploaderBundle and JMSDiExtraBundle are doing something "awful" we should ask ourselves: Why are they doing something awful? Is this a bundle problem or a Symfony shortcoming? Is there an alternative solution that we can propose to those bundle creators? Thanks!

I 1000℅ agree with@javiereguiluz, that's also my point since the beginning!

Could we at least log a warning in the ContainerBuilder logs when using such thing ?

I just want to be sure about the initial issue. If someone try toget a service in aBundleExtension, or in aCompilerPass, he could get an error because the service is not fully initialized. And you said it's an issue because it's hard to spot the root cause of the error?

I propose to throw a different exception/message when someone try to get a service (or the DIC itself when it solves dependencies)and the DIC is not compiled yet. Something like "The service XXX does not exist or is not available yet because the service container is not fully compiled yet".

WDYT?

@lyrixx Well, that was the initial idea and why we deprecated callingget() before the container has been compiled so that we could actually throw an exception in 4.0. But that led to issues like the one in VichUploaderBundle linked above. Thus, we cannot do more right now than at least write something to the log if a service is fetched before the container has been compiled.

@lyrixx the issue is that the service definition may exist yet, but in a broken state, because making it working requires the work done in a compiler pass which has not been executed yet.
All issues reported here are not about a service not existing, but about a service definition being incomplete and so the instantiation failing.

Sorry, I will clarify. I propose to throw this message if and only if a service is not available yet. If everything goes fine, it's fine. Not message.

@stof I was not aware of this. How could a definition be broken? Like a missing arg not yet replaced?

an argument being missing, the autowiring not being applied yet (which will generally trigger the case of a missing arg), an argument not being replaced yet (and so for instance having an empty list of Twig extensions in the Twig_Environment, or getting the event dispatcher with no listener registered in it), a decorated service not being decorated yet (this one is tricky to debug, because it willnot fail, but will use the original service without decorating it). Look at everything done in compiler passes, and imagine getting the service without applying this logic (which is what happens).

and if the service is not there yet, there is already a clear exception message saying that the service does not exist. This is not the case being hard to debug (thus, it is uncommon to create services from scratch in compiler passes. Most compiler passes are altering service definitions created by the DI extension)

I understand the issue here. But clearly, it's so useful to get some service in a BundleExtension. For example in BF, we have a service to validate the.blackfire.yml. And we use it during the kernel::boot (inside the BundleExtension) to validate our default configuration (metrics, tests, recommendations etc). Plugin everything by hand is a bit cumbersome !

@lyrixxKernel::boot may not be in the container building phase anymore depending of how you hook into it. Using a service inAppBundle::boot is totally fine for instance, as the container compilation is done at this point.
If you hook intoAppKernel::boot, it depends whether you do it before or after the parent call (as theKernel::boot is the place callingKernel::initializeContainer to compile the container)

The compiler pass should indicate which services it would like to have access to and which service it wants to modify (this can be deduced automatically when you parse but not execute the php code, but is more difficult). Then the order of calling compiler passes should be checked with what is already "stable" in the container. You will then get the notion of groups of services that are already "stable", after each group is stable you can run compiler passes with it, on services that are yet to become stable. It's the only way to really solve this problem. This requires to solve the dependency graph of the compiler passes (the functions) and the services (the data). A certain set of services will be like a type that your compiler pass requires, figuring out the "type" on this level is a matter of checking which services have already become stable (don't have any more compiler passes to do on them). You then get type safety when right now we are still in undefined behavior land. Just to make clear, when i mention functions, data and types i mean it on the level of the "services DSL" (= the language in which you describe which services you have and which compiler passes you have and what to execute in which order) and not native PHP functions, data or types. After all we are talking about the container compiler and not the php compiler.

@flip111 the issue is that a change done by a compiler pass can change the fact that a later compiler pass relies on this service or no

@stof don't really see what's the issue is about that, you just have to consider your service not to be the service you need after you changed it. Soservice A becomesservice A_modified, then in your compiler pass you depend on either one of them. This is the step what the compiler has to dohttps://en.wikipedia.org/wiki/Static_single_assignment_form Of course you need to know in which order your compiler passes are going to execute.

@flip111 but this means that you cannot determine the service affected by a compiler pass without knowing the state of the container at the time this pass is called. So this does not allow you to define safe services for earlier compiler passes.

@stof I don't really understand your last sentence. About the first sentence: the state of the container is known since you know which compiler passes already have been performed.

@flip111 knowing which services are safe when running the compiler pass A depends on whether a later compiler pass will change its config or no. But we cannot know this without running the compiler pass A (as knowing which service are affected by B requires knowing the state of the container at the time B is called, i.e. after applying A).
So this means you have to be able to execute A to have a chance to know whether it was safe for it to access a service during its execution...

@stof

knowing which services are safe when running the compiler pass A depends on whether a later compiler pass will change its config or no

I interpret this as "if a later compiler pass will change a service then it was not safe to use this service in compiler pass A". I think this is true, at the same time if a later compiler pass changes some things in a service which are not needed in compiler pass A then it's not a problem that your service is not stable yet.

as knowing which service are affected by B requires knowing the state of the container at the time B is called

You can know which services are affected by B if you define this somewhere. This is what i meant with "The compiler pass should indicate which services it would like to have access to and which service it wants to modify". You could say that .. well sometimes compiler pass A is not going to actually modify that service (so optionally modify). So compiler pass B gets eitherservice A orservice A modified by compiler pass A, then you leave it up to the programmer to make sure that compiler pass B can accept both variations of the same service. But now you have safety that you oblige the programmer to handle both cases.

So this means you have to be able to execute A to have a chance to know whether it was safe for it to access a service during its execution...

So suppose you can first run your compiler, then figure out what went wrong, then re-run it until you know for sure what's going to happen. That would be one way, another way would be to solve it with symbols. Example you have a symbol forservice A and a functioncompiler pass A that producesservice A modified by compiler pass A, if you have this information available beforehand then you don't have to run the actual php code to figure out what will go wrong, because you can already determine it upfront and tell whether the chain of compiler passes is going to be safe or not.

You can know which services are affected by B if you define this somewhere. This is what i meant with "The compiler pass should indicate which services it would like to have access to and which service it wants to modify".

but the whole point of compiler passes is that theycannot know which service they modify without knowing the state of the container, as the main use case for compiler passes is to collect services defined by any bundle based on tags.

So suppose you can first run your compiler, then figure out what went wrong, then re-run it until you know for sure what's going to happen. That would be one way, another way would be to solve it with symbols. Example you have a symbol for service A and a function compiler pass A that produces service A modified by compiler pass A, if you have this information available beforehand then you don't have to run the actual php code to figure out what will go wrong, because you can already determine it upfront and tell whether the chain of compiler passes is going to be safe or not.

the more I read your answers, the more I think you don't understand at all what compiler passes are in the Symfony DIC and what they are used for.

@stof

Let's put it this way: do you think with careful human inspection of every line of code you can make sure that the compiler chain is safe for a given set of services and compiler passes? If this is not true then you are right and i don't understand the problem. If this is true then what makes you think this can not be automated?

@flip111 I can only do it by understanding what the final service configuration should look like. Without that knowledge, I cannot tell at which point the service definition would be ready. And knowing this requires understanding the intent of the developr and the way the container is meant to look like based on the config. If this could be automated, we would not need to write any compiler pass anymore, as it would mean that the framework code would know more about how your project works than any developer working on any part of the framework or the project (quite hard to achieve...)

Once I know this, I could indeed determine at which point the service will be ready, by determining changes performed by compiler passes. but determining this would still be a nightmare when an actual service gets instantiated in a compiler pass, as making sure it can be instantiated properly can quickly become a chicken-and-egg issue again.
And determining all this would require understanding what changes are applied on the container by each compiler pass exactly. I generally am not able to achieve it without actually running some code with a debugger, to determine an intermediate state (I could do it fully by hand, but it would take a lot of time, and a very deep knowledge of Symfony that most devs don't have).
Automating it would actually mean replaying the whole execution of the PHP code building the container without actually running it, to determine its effect and tracing at which point we encounter the chicken-and-egg issue. This would be totally insane.

@stof I was exactly talking about that "replaying the execution without actually running the code". I'm pretty sure to solve the graph of services and compiler passes you will need to do control flow analysis. My point merely was that i belief if you want to solve this problem then this will be your only path. A lot of work? Definitely. Insane? i don't think so ... this is standard compiler technology.

@flip111 but compiler passes can contain any PHP code

@stof true, but when the programmer who understands his own services and compiler passes best tells you
"this compiler pass requires service A after it's been modified by compiler pass A" then we must trust that. You don't actually have to go down to the absolutely lowest level of execution and you can solve it on a higher level with symbols.

@flip111 but I'm unable to tell you which service myown compiler passes are modifying in your project, as it depends of the state of the container in your project, and so depends on all bundles you installed and of the config done in your project.
for instance, the issue in VichUploaderBundle which started this discussion depends on many things:

• if you change the annotation caching, the VichUploaderBundle compiler pass gets broken or no
• once my PR to the bundle is merged or no, the bug appears only when you have a propel mapping configured in the bundle
• it could also depend on whether you enabled annotation mapping in the driver or no if it was configurable
• it could also depend on extra drivers

And they cannot say "my compiler pass requires service A after it's been modified by compiler pass A". It can only say "my compiler pass requires service A". Knowing which compiler pass is necessary for the service to be in a working state requires knowing which features are used by the service definition, and which compiler passes are involved in these features, as well as compiler passes dealing with tags the service has. And then, it cascades to all dependencies of this service, as instantiating a service then needs to be able to instantiate the dependencies (which is what happens for VichUploaderBundle). And as compiler passes are precisely about changing the container config (if you look at compiler passes, you will see that almost all of them are changing the dependency graph), it still requires tracing down all changes.
Thus some features cannot even be detected by looking at the service definition of A, but by looking at other services (service decoration for instance)

@stof As i look at this filehttps://github.com/dustin10/VichUploaderBundle/blob/master/DependencyInjection/Compiler/RegisterPropelModelsPass.php i see it requires the servicevich_uploader.metadata_reader and does not change any instances of a service. Am i missing something here? The stress was on line 30 if it was safe to getvich_uploader.metadata_reader yes or no.

@flip111 that's not true. A few line belows, it also alters some service definition.

and the point is that determining whethervich_uploader.metadata_readerand all its dependency graph must be ready to be instantiated at this point. And this is what is almost impossible, as determining this is totally dependant of what happens in each compiler pass and of what happened before for the state before compilation.

@stof

I thought changing service definitions was a safe alternative to getting a service instance. I got this idea from reading

As a rule, only work with services definition in a compiler pass and do not create service instances. In practice, this means using the methods has(), findDefinition(), getDefinition(), setDefinition(), etc. instead of get(), set(), etc.

From:https://symfony.com/doc/current/components/dependency_injection/compilation.html

If we only talk about service instances i can deduce the following:

• metadata_reader depends onmetadata_factory
• metadata_factory depends onmetadata_driver
• metadata_factory depends optional onmetadata.cache, since the service works without it we can forget about this one.
• metadata_driver depends onmetadata_driver.annotation,metadata_driver.yaml,metadata_driver.xml
• metadata_driver.annotation depends onannotation_reader
• metadata_driver.yaml andmetadata_driver.xml depend onmetadata.file_locator

https://github.com/dustin10/VichUploaderBundle/blob/0ecb31a3f5b1c64931346a187284221abfa107a3/Resources/config/mapping.xml

No other compiler passes are used in the bundle. That leaves possible other compiler passes by the bundle user. You can choose to not allow any compiler passes, all compiler passes must be performed or don't care. As for the special case of bundles you only have these options. In a normal app you have knowledge about which other compiler passes there are. Actually you also have knowledge of outside the bundle, but only on code that is required in your composer file.

@flip111 Many features in the component itself are relying on compiler passes too. Using the container without applying compiler passes does not work.

And you forget all dependencies ofannotation_reader, which is where the cache pool will appear (and this cache pool relies on service definition inheritance, which is resolved by a compiler pass)

PR with similar requirementsdoctrine/DoctrineBundle#658 . Have anybody idea how it possible make things in this PR without getting services from uncompiled container?