-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[RFC][DependencyInjection][HttpKernel] Introduce a contextualized ContainerBuilder & BootingKernel #19646
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
Conversation
|
||
public function get($key) | ||
{ | ||
if (!isset($this->elements['key'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ImmutableKernel
? (it can be reused in many ways).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we implement TerminableKernelInterface
for convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this phpdoc needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #19646 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of suspected it but isn't this more a problem on phpStorm side than anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 For $elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unserialize does not have any return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ContainerBuilder
should probably not behaves as aContainer
until 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.
Context
instance 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()
?Context
class immutable (Thus cannot be changed after creating theContainerBuilder
instance) ? 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 aBootingKernel
instance.