[DI] Revert "deprecate get() for uncompiled container builders" #20533
Conversation
|
👍 for the revert. |
|
Thank you for this fix :) 👍 |
7569f7d to
7f32e6d
Compare
|
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). |
|
-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. 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 the
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 have no 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? Im not even sure we can do both actually :)) |
7f32e6d to
e449af0
Compare
|
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. |
…builders" (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [DI] Revert "deprecate get() for uncompiled container builders" | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18728 | License | MIT | Doc PR | - ping @xabbuh This reverts commit 27f4680, reversing changes made to e4177a0. While upgrading a few projects to 3.2, I found it impossible to remove this deprecation without duplicating "by hand" the instantiation logic of some service needed in a compiler pass. See https://github.com/dustin10/VichUploaderBundle/blob/master/DependencyInjection/Compiler/RegisterPropelModelsPass.php#L30 for such an example into the wild. I think we should revert this deprecation. Using the container builder before it is compiled is tricky. Yet, bootstrapping in general is tricky and full of chicken-and-egg issues. In this case, using some services while bootstrapping the container works well. Deprecating would be fine if we were to provide some viable alternative. Duplicating the instantiation logic doesn't qualify as such to me, hence the proposed revert. @xabbuh, if you'd like to keep some test cases that may be unrelated to the revert, please add comments on the PR, or better: push on my fork, branch revert-get-deprec. Commits ------- e449af0 Revert "feature #18728 deprecate get() for uncompiled container builders (xabbuh)"
|
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 to 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 calling |
|
@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. |
|
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 will not 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 |
|
@lyrixx |
|
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 A3E2 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. So |
|
@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). |
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.
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 either
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 |
but the whole point of compiler passes is that they cannot 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.
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. |
|
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. |
|
@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 |
|
@flip111 but I'm unable to tell you which service my own 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.
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. |
|
@stof As i look at this file https://github.com/dustin10/VichUploaderBundle/blob/master/DependencyInjection/Compiler/RegisterPropelModelsPass.php i see it requires the service |
|
@flip111 that's not true. A few line belows, it also alters some service definition. and the point is that determining whether |
|
I thought changing service definitions was a safe alternative to getting a service instance. I got this idea from reading
From: https://symfony.com/doc/current/components/dependency_injection/compilation.html If we only talk about service instances i can deduce the following:
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 of |
|
PR with similar requirements doctrine/DoctrineBundle#658 . Have anybody idea how it possible make things in this PR without getting services from uncompiled container? |
ping @xabbuh
This reverts commit 27f4680, reversing
changes made to e4177a0.
While upgrading a few projects to 3.2, I found it impossible to remove this deprecation without duplicating "by hand" the instantiation logic of some service needed in a compiler pass.
See https://github.com/dustin10/VichUploaderBundle/blob/master/DependencyInjection/Compiler/RegisterPropelModelsPass.php#L30 for such an example into the wild.
I think we should revert this deprecation. Using the container builder before it is compiled is tricky. Yet, bootstrapping in general is tricky and full of chicken-and-egg issues. In this case, using some services while bootstrapping the container works well. Deprecating would be fine if we were to provide some viable alternative. Duplicating the instantiation logic doesn't qualify as such to me, hence the proposed revert.
@xabbuh, if you'd like to keep some test cases that may be unrelated to the revert, please add comments on the PR, or better: push on my fork, branch revert-get-deprec.