8000 [RFC] How to deprecate ContainerBuilder::get before compilation, again? · Issue #20643 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[RFC] How to deprecate ContainerBuilder::get before compilation, again? #20643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ro0NL opened this issue Nov 26, 2016 · 24 comments
Closed

[RFC] How to deprecate ContainerBuilder::get before compilation, again? #20643

ro0NL opened this issue Nov 26, 2016 · 24 comments
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@ro0NL
Copy link
Contributor
ro0NL commented Nov 26, 2016

So, back at square one i guess :)

#20533 reverted the integrity check for accessing services, before the container builder is actually compiled. Making it real tricky, yet convenient. However, it is done for the right reasons, for now.

Where does that leave us? Will it be put back? And what is the alternative/right approach?

Imo. we all agree the check is needed to keep the container trusty, but we lose major flexibility on simple services (ones that can actually be resolved fully at some point before compilation).

Some ideas;

  • (1) Keep it as is. If this was already decided by now, please close :) Basically saying to the user; with great power, comes great responsibility; do as you like / whatever works for you. Until it breaks that is :))
  • Deprecate it!
    • (2) Teach the user to do a different approach in user-land (ie. just initialize a service hardcoded, where needed, when needed). As there will be many tickets about this i guess.
    • (3) Introduce a 2nd container, compiled and accessible, before compiling the 1st container. The user can configure required services as usual. Ie. what about tagging required services with bootstrap, making the full service available on booting, ie. one config and no duplicate service definitions.
    • (4) Allow the user to get a service without compiling the current container (ie. clone it, compile it, then get it). However this basically acts the same as option 1 (keeping it as is), whenever it breaks / doesnt work anymore you still need to revise your approach.
    • (5) Detect which services are safe, and which arent out-of-the-box (the holy grail).
    • (6) Allow compiler passes to run last with scope on a (sub)compiled version of the container, still allowing to modify the actual container. Doing it in 2 steps, the other way around.
@linaori
Copy link
Contributor
linaori commented Nov 26, 2016

Teach the user to do a different approach in user-land (ie. just initialize a service hardcoded, where needed, when needed). As there will be many tickets about this i guess.

This would be my choice. In 3 years time with Symfony 2 and up, I've done some spooky things, but I've never needed services at compile-time, there's always another way around it. This is the same advice I always give people on IRC.

@javiereguiluz javiereguiluz added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Nov 26, 2016
@ro0NL
Copy link
Contributor Author
ro0NL commented Nov 26, 2016

Added an extra option (5), coming from an ongoing discussion in #20533

I tend to go with option 2 as well, however it could be cool to allow configuring those services somehow using a DI container.

For example it allows tagging compiler passes.. 😱 how cool is that?

@nicolas-grekas
Copy link
Member

I'm 👎 for this deprecation. I don't now if we can find some bootstrapping procedure that could allow to safely get some early services to help building the next ones. But for now, I'm really fine with this sentence: "with great power, comes great responsibility".

@ro0NL
Copy link
Contributor Author
ro0NL commented Nov 27, 2016

To be honest, for the DI component i find it a no-brainer adding it.

some bootstrapping procedure that could allow to safely get some early services

The DI component does exactly that (well.. it's not really "safe" at the moment ;-)..., ie. i think doing it in 2 steps makes sense. In theory we could initialize the actual container from the booting container (having a container as a service). Endless possibilities..

@xabbuh
Copy link
Member
xabbuh commented Nov 27, 2016

One idea I have in mind is making it possible to extract a subset from the DI graph. That is ask the container builder for a definition and all its dependencies. This subset (sub-container) could be compiled independently from the passed container builder and the resulting container could then be used to call get() while leaving the initial container builder untouched. If we made that approach easy to use, we should be safe deprecating calling get() before compilation again.

@linaori
Copy link
Contributor
linaori commented Nov 27, 2016

I guess one way could be instead of extending the container, simply decorate it (once done). A few benefits I see here:

  • The incomplete state would never enter the actual container, thus consistent behavior between the first and second request of an application if the first is doing a build
  • What @xabbuh suggested could be a container builder only feature which doesn't even use the container
  • Once build, the kernel container could be replaced by the actual container hidden in the builder

@ro0NL
Copy link
Contributor Author
ro0NL commented Nov 27, 2016

Imo. we should not rely on a container that is being / going to be build. Resolving dependencies, means it must be compiled, means we're done. Compiling a sub graph may still be an incomplete sub graph from the user pov.

We should not mix&match imo.

@HeahDude
Copy link
Contributor

Why not using a new constant of PassConfig to register passes that are run at last?

@ro0NL
Copy link
Contributor Author
ro0NL commented Nov 27, 2016

Added to the list. Could actually work 👍

@HeahDude
Copy link
Contributor
HeahDude commented Nov 27, 2016

that are run at last

Those type of passes should be limited in the way they can modify the current state or another process of optimization should run (virtually two containers would be built optimized).

@javiereguiluz
Copy link
Member

A while ago I had this problem in a bundle that used compiler passes (since then I replaced the compiler pass with a cache warmer and the problem disappeared). In any case, at that time, the proposal by @HeahDude is exactly what I needed: give me the chance to do things in the container after Symfony has finished all its work and just before the container is frozen.

@linaori
Copy link
Contributor
linaori commented Nov 27, 2016

I think that pass might actually work out

@GuilhemN
Copy link
Contributor

@HeahDude shouldn't using PHP_MAX_INT as priority be enough?

@HeahDude
Copy link
Contributor

@GuilhemN the way I see it, it's not enough to determine internally if the pass is able to call get safely without triggering a deprecation notice or an exception.

@GuilhemN
Copy link
Contributor
GuilhemN commented Nov 27, 2016

@HeahDude i get it, looks legit :)

Those type of passes should be limited in the way they can modify the current state or another process of optimization should run (virtually two containers would be built optimized).

To make it even safer, my proposal would be: when a service is fetched in an uncompiled container, everything related to the service should be "locked" (can't be updated anymore), that's near @xabbuh's proposal but simpler to implement as it's more radical. get should only be called during the phase proposed by @HeahDude or other passes may try to update locked definitions, which would throw an exception.
Conflicts might happen between passes even during this phase (a pass triing to update a definition that was locked by another pass) but priorities were introduce to deal with them :)
Wdyt?

If this satisfies us, we could even prevent calling get before the last compilation phase.

@flip111
Copy link
Contributor
flip111 commented Nov 28, 2016

@xabbuh this is in another wording what i was trying to discuss in #20533 (comment) . I think i might have not expressed that very well because (5) Detect which services are safe sounds very sophisticated, it was more meant as define which compiler pass does what to making it possible to extract a subset from the DI graph.

Before making a final decision on the given options it would be good to have an overview of what can go wrong in the various scenario's. @stof gave some good info on this already.

@stof
Copy link
Member
stof commented Nov 28, 2016

@GuilhemN freezing definitions would not solve the issue that you can get weird failures when getting a service which is not ready yet.
And this could make things hard as getting a service early might freeze lots of services, preventing optimizing the container or even resolving them (what if you freeze a service before applying the service decoration on it ?). It opens the door for badly-written compiler passes to break all other compiler passes instead of only breaking themselves.

Extracting a subset of the DI graph would probably not help, as compiling this subset fully would involve running all compiler passes again, which may trigger an infinite loop when reaching the compiler pass calling the service, and defining the subset itself is still as hard considering that the graph changes during each compiler pass (or can change at least, even if some compiler passes don't change it).

@GuilhemN
Copy link
Contributor
GuilhemN commented Nov 28, 2016

@stof that's why freezing services should only be done after symfony's work.
If a service isn't ready yet when getting it, you'll either have an exception thrown by it (might be a weird failure i agree but i don't think we can do something about that even with other solutions) or thrown by a pass triing to update it later: so a container based on a service that was fetched while not ready won't compile.

Do you think it's necessary to support fetching services before symfony's work? It looks very unsafe to me.

@stof
Copy link
Member
stof commented Nov 28, 2016

@GuilhemN but most compiler passes run before Symfony's work, as adding new dependencies in the graph must be done before optimizing the graph (otherwise, optimizations would break things).
In most projects, the Symfony compiler passes are the last ones to run.

@ro0NL
Copy link
Contributor Author
ro0NL commented Nov 28, 2016

Not sure, how i see @HeahDude's approach;

// before calling the "last" passes
$container = new ContainerBuilder();
$container->merge($actualContainer);
$container->compile();

// further modify $actualContainer, while allowed to get() from $container

// compile $actualContainer as usual

Meaning we probably need an additional pass interface, right? Ie. FinalizingCompilerPassInterface::getContainer() : ContainerInterface

8000

@stof
Copy link
Member
stof commented Nov 28, 2016

@javiereguiluz many compiler passes I saw getting services are using them to configure other services, and so they cannot run after Symfony's work.

If you want to run after everything was done in the container compilation, this indeed looks like a use case for a cache warmer rather than a compiler pass.

@GuilhemN
Copy link
Contributor

@stof that's a fair point.
We could optimize parts of the container before freezing them to allow using get before Symfony's work but I don't think that's doable without modifying the concept of optimization passes... let's see if other proposals are simpler to implement :)

@stof
Copy link
Member
stof commented Nov 29, 2016

@GuilhemN we simply cannot optimize the container earlier, as optimization relies on knowing the dependency graph to identify things which can be inlined or removed.
Once optimization passes are done, the only change you can do safely is removing stuff. If you add anything (adding an argument to a service, adding a new service referencing another service, etc...) is likely to break if the referenced service was involved in an optimization.

@ro0NL
Copy link
Contributor Author
ro0NL commented Feb 8, 2017

Option 1 it is i guess :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

9 participants
0