[RFC][DependencyInjection][HttpKernel] Introduce a contextualized ContainerBuilder & BootingKernel #19646
Conversation
|
|
||
| public function get($key) | ||
| { | ||
| if (!isset($this->elements['key'])) { |
There was a problem hiding this comment.
Typo
+ if (!isset($this->elements[$key])) {
- if (!isset($this->elements['key'])) {|
I think extensions built the container and (optionally) read the context, the context is global and definitive at this point. So yeah, i favor immutable. |
|
I confirm it fixes #18563 (manually checked it out). |
| /** | ||
| * @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
| */ | ||
| class BootingKernel implements KernelInterface |
There was a problem hiding this comment.
What about ImmutableKernel? (it can be reused in many ways).
There was a problem hiding this comment.
Why not, but...
I was wondering if we should explicit the kernel state at this point in the class name. I finally tend to say yes, because it's also an unusable kernel instance. I.e you cannot use handle(). So ImmutableKernel is true, but not enough.
What do you think ?
There was a problem hiding this comment.
True, true :) lets be pragmatic; ImmutableKernel will cover, imo. Perhaps add a constructor argument $allowHandling = false.
Also consider implementing TerminableKernelInterface and check if $innerKernel implements it (then consider $allowTerminating = false in the constructor).
edit: i understand your hesitation.. guess a (specific) BootingKernel is fine.
There was a problem hiding this comment.
it's a kernel decorator, so should we call it KernelDecoratorKernel? To me looks ugly and I wouldn't name a class after a pattern. ImmutableKernel wouldn't make sense if other kernel were immutable as well, it's just a way to implement something it does not express what is specific about this class. BootingKernel makes more sense
There was a problem hiding this comment.
Should we implement TerminableKernelInterface for convenience?
There was a problem hiding this comment.
No, it makes no sense. the decorated kernel can be a TerminableInterface instance, but we'll never call terminate(Request $request, Response $response) from a booting kernel. It's read-only, and moreover cannot finish a request as it can't even handle one ! 😅
There was a problem hiding this comment.
Agree. Just thought of it when used as a drop-in replacement for the current instance we have, but it makes no sense. It's not meant to be a drop-in replacement.
| private $compiled = false; | ||
|
|
||
| /** | ||
| * @param ParameterBagInterface|null $parameterBag A ParameterBagInterface instance | ||
| * @param Context|null $context A Context instance, or null |
There was a problem hiding this comment.
See #19646 (comment)
There was a problem hiding this comment.
erm, I still fail to see what is in the phpdoc that is not in the signature, new parameter or not. Or I am missing something?
There was a problem hiding this comment.
Just as I did, you're missing this:
😅
There was a problem hiding this comment.
I kind of suspected it but isn't this more a problem on phpStorm side than anything?
There was a problem hiding this comment.
I mentioned it, because of https://www.phpdoc.org/docs/latest/guides/inheritance.html
AFAIK docblock inheritance happens by default from a phpdoc pov.
because a DocBlock for a class makes full use of the object-oriented principles that PHP offers and inherits the following information from the superclass
Ie. you'd inherit the wrong @param doc from Container::__construct. @inheritdoc is optional, however using it can be seen a best practice;
it does make clear that an element has been explicitly documented (and thus not forgotten)
But using it inline is plain wrong 😄 (again from phpdoc pov).
edit: if we remove the docblock from Container::__construct, we can remove it here. And rely on method signatures in both cases i guess.
edit2: so phpstorm is doing it right actually 😉
|
I've pushed a new commit to make the context immutable. |
| * | ||
| * @return Context | ||
| */ | ||
| public static function create(array $contextElements = array(), Context $fromContext = null) |
There was a problem hiding this comment.
What about array $elements = array(), Context $fromContext = null
It doesnt repeat terms, that are obvious due the context of this class.
There was a problem hiding this comment.
I've renamed the $elements argument after #19646 (comment).
But clearly, to me, the description is now enough.
About renaming $fromContext to $previous, why not. But, there is two schools of thought:
create(array $elements = array(), Context $previous = null)
Straightforward, no repetition.create(array $withElements = array(), Context $fromContext = null)
It reads well:Create a context with given elements, from context.
What do you think ?
There was a problem hiding this comment.
I would favor the first ($previous vs $fromContext is dubious), the repetition feels too much boilerplate.
| */ | ||
| public function unserialize($serialized) | ||
| { | ||
| return $this->innerKernel->unserialize($serialized); |
There was a problem hiding this comment.
unserialize does not have any return value
There was a problem hiding this comment.
Thanks @Nicofuma :)
|
I'm closing this PR. I'm not convinced that it solves a problem that needs to be solve by code. |
|
No problem, I agree. However, some issues like #18563 still need a solution. |
Supersedes #19606 by giving another more generalist approach.
It follows discussions in #19619.
As the
ContainerBuildershould probably not behaves as aContaineruntil compiled (see #19644) and as services set directly usingContainerBuilder::set()are vanished and cannot be got during the building phase, this POC exposes the idea of separating clearly the needs of using a service and sharing a context somehow while the container is building.Apparently, it should help to fix issues like #18563 which needs to access the bundle instances during the extensions loading.
Contextinstance be locked before or after extensions are loaded ?I.e: Should we allow extensions to mutate the context and consider it "safe to use" only in compiler passes, or should we only allow to initially set the context from the
KernelInterface::getContainerBuilder()?Contextclass immutable (Thus cannot be changed after creating theContainerBuilderinstance) ? No morelock, no moreset.Another solution only focused on the need to access the kernel & bundles instances from an extension could be to create a
KernelAwareInterface. If an extension implements this interface, then we inject aBootingKernelinstance.