8000 [RFC][DependencyInjection][HttpKernel] Introduce a contextualized ContainerBuilder & BootingKernel by ogizanagi · Pull Request #19646 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 3 commits into from
Closed

Conversation

ogizanagi
Copy link
Contributor
@ogizanagi ogizanagi commented Aug 17, 2016
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Supersedes #19606 by giving another more generalist approach.
It follows discussions in #19619.

As the ContainerBuilder should probably not behaves as a Container until compiled (see #19644) and as services set directly using ContainerBuilder::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.

  • Useful ? Is the approach legit ?
  • Should the 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() ?
  • If the answer to the previous question is "Do not allow extensions to mutate the context", what about simply making the Context class immutable (Thus cannot be changed after creating the ContainerBuilder instance) ? No more lock, no more set.
  • Add tests if approved.

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 a BootingKernel instance.

@carsonbot carsonbot added Status: Needs Review RFC RFC = Request For Comments (proposals about features that you want to be discussed) DependencyInjection HttpKernel Feature labels Aug 17, 2016

public function get($key)
{
if (!isset($this->elements['key'])) {
Copy link
Member

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'])) {

@ro0NL
Copy link
Contributor
ro0NL commented Aug 18, 2016

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.

@chalasr
Copy link
Member
chalasr commented Aug 18, 2016

I confirm it fixes #18563 (manually checked it out).
This approach looks lightweight and simple (which is 👍 as use cases are not so many but totally blocked ATM) and should not be hard to maintain, so IMHO good to have.

/**
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com>
*/
class BootingKernel implements KernelInterface
Copy link
Contributor
@ro0NL ro0NL Aug 18, 2016

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).

Copy link
Contributor Author
@ogizanagi ogizanagi Aug 18, 2016

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 ?

Copy link
Contributor
@ro0NL ro0NL Aug 18, 2016

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.

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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 ! 😅

Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this phpdoc needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as I did, you're missing this:

capture d ecran 2016-08-18 a 16 46 00

😅

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure 😕

Copy link
Contributor
@ro0NL ro0NL Aug 18, 2016

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 😉

@ogizanagi
Copy link
Contributor Author

I've pushed a new commit to make the context immutable.

*
* @return Context
*/
public static function create(array $contextElements = array(), Context $fromContext = null)
Copy link
Contributor
@ro0NL ro0NL Aug 19, 2016

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 For $elements

Copy link
Contributor Author
@ogizanagi ogizanagi Aug 19, 2016

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 ?

Copy link
Contributor

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);
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Nicofuma :)

@fabpot
Copy link
Member
fabpot commented Nov 9, 2016

I'm closing this PR. I'm not convinced that it solves a problem that needs to be solve by code.

@fabpot fabpot closed this Nov 9, 2016
@ogizanagi
Copy link
Contributor Author

No problem, I agree.

However, some issues like #18563 still need a solution.

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

Successfully merging this pull request may close these issues.

8 participants
0