-
-
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
[RFC][DependencyInjection][HttpKernel] Introduce a contextualized ContainerBuilder & BootingKernel #19646
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
use Symfony\Component\Config\Resource\ResourceInterface; | ||
use Symfony\Component\DependencyInjection\LazyProxy\Instantiator\InstantiatorInterface; | ||
use Symfony\Component\DependencyInjection\LazyProxy\Instantiator\RealServiceInstantiator; | ||
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface; | ||
use Symfony\Component\ExpressionLanguage\Expression; | ||
use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface; | ||
|
||
|
@@ -33,7 +34,7 @@ | |
* | ||
* @author Fabien Potencier <fabien@symfony.com> | ||
*/ | ||
class ContainerBuilder extends Container implements TaggedContainerInterface | ||
class ContainerBuilder extends Container implements TaggedContainerInterface, ContextualizedContainerBuilderInterface | ||
{ | ||
/** | ||
* @var ExtensionInterface[] | ||
|
@@ -89,8 +90,21 @@ class ContainerBuilder extends Container implements TaggedContainerInterface | |
*/ | ||
private $usedTags = array(); | ||
|
||
private $context; | ||
|
||
private $compiled = false; | ||
|
||
/** | ||
* @param ParameterBagInterface|null $parameterBag A ParameterBagInterface instance | ||
* @param Context|null $context A Context instance, or null | ||
*/ | ||
public function __construct(ParameterBagInterface $parameterBag = null, Context $context = null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This requires a new docblock. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure as it won't bring anything. Appart if we need to add a description. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You add a new parameter. /**
* {@inheritdoc}
*
* @param ParameterBagInterface|null $parameterBag A ParameterBagInterface instance
* @param Context|null $context A Context instance, or null
*/ And perhaps also fix it for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. Code updated. Thank you @ro0NL :) |
||
{ | ||
parent::__construct($parameterBag); | ||
|
||
$this->context = $context ?: Context::create(); | ||
} | ||
|
||
/** | ||
* Sets the track resources flag. | ||
* | ||
|
@@ -1017,6 +1031,14 @@ public static function getServiceConditionals($value) | |
return $services; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getContext() | ||
{ | ||
return $this->context; | ||
} | ||
|
||
/** | ||
* Retrieves the currently set proxy instantiator or instantiates one. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\DependencyInjection; | ||
|
||
use Symfony\Component\DependencyInjection\Exception\ContextElementNotFoundException; | ||
|
||
/** | ||
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
*/ | ||
final class Context | ||
{ | ||
private $elements; | ||
|
||
private function __construct(array $elements = array()) | ||
{ | ||
$this->elements = $elements; | ||
} | ||
|
||
/** | ||
* Creates a new context allowing to share elements during the container building phase. | ||
* | ||
* Context elements is an array where the values are the elements to share | ||
* and the keys are strings allowing to retrieve an element. | ||
* | ||
* For instance: | ||
* | ||
* Context::create(array('kernel' => new BootingKernel($kernel))); | ||
* | ||
* You can optionally pass an existing Context instance. Thus, original elements are reused, | ||
* but new context elements replace any existing key. | ||
* | ||
* @param array $elements An array of elements indexed by a string. | ||
* @param Context|null $previous An optional Context instance used as base. | ||
* | ||
* @return Context | ||
*/ | ||
public static function create(array $elements = array(), Context $previous = null) | ||
{ | ||
return new self($previous ? array_replace($previous->elements, $elements) : $elements); | ||
} | ||
|
||
/** | ||
* @param string $key | ||
* | ||
* @return bool | ||
*/ | ||
public function has($key) | ||
{ | ||
return isset($this->elements[$key]); | ||
} | ||
|
||
/** | ||
* @param string $key | ||
* | ||
* @return mixed | ||
* | ||
* @throws ContextElementNotFoundException When no element exists for this key. | ||
*/ | ||
public function get($key) | ||
{ | ||
if (!$this->has($key)) { | ||
throw new ContextElementNotFoundException($key); | ||
} | ||
|
||
return $this->elements[$key]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\DependencyInjection; | ||
|
||
/** | ||
* Implemented by a ContainerBuilder sharing a context during build time. | ||
* | ||
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
*/ | ||
interface ContextualizedContainerBuilderInterface extends ContainerInterface | ||
{ | ||
/** | ||
* @return Context | ||
*/ | ||
public function getContext(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\DependencyInjection\Exception; | ||
|
||
/** | ||
* This exception is thrown when a non-existent context element is used. | ||
* | ||
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
*/ | ||
class ContextElementNotFoundException extends InvalidArgumentException | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dont get me wrong ;-) but consider extending There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I've reflected what is done in the The argument format is valid, but the argument is not valid in this context. Also that probably means it should lead to a change in your code, so it must be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets keep it. This is hard to explain (my pov) for PHP; everything is runtime, and e 10000 verything is a programmers fault too eventually. I went down that road long time ago, https://stackoverflow.com/questions/14171714/php-runtime-or-logic-exception |
||
{ | ||
private $key; | ||
|
||
/** | ||
* @param string $key | ||
*/ | ||
public function __construct($key) | ||
{ | ||
$this->key = $key; | ||
|
||
parent::__construct(sprintf('Context element not found for "%s" key', $key)); | ||
} | ||
|
||
/** | ||
* @return string | ||
*/ | ||
public function getKey() | ||
{ | ||
return $this->key; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\HttpKernel; | ||
|
||
use Symfony\Component\Config\Loader\LoaderInterface; | ||
use Symfony\Component\DependencyInjection\Exception\BadMethodCallException; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpKernel\Bundle\BootingBundle; | ||
use Symfony\Component\HttpKernel\Bundle\BundleInterface; | ||
|
||
/** | ||
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
*/ | ||
class BootingKernel implements KernelInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 What do you think ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, true :) lets be pragmatic; Also consider implementing edit: i understand your hesitation.. guess a (specific) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's a kernel decorator, so should we call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it makes no sense. the decorated kernel can be a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 $innerKernel; | ||
|
||
public function __construct(KernelInterface $innerKernel) | ||
{ | ||
$this->innerKernel = $innerKernel; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function serialize() | ||
{ | ||
return $this->innerKernel->serialize(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function unserialize($serialized) | ||
{ | ||
$this->innerKernel->unserialize($serialized); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = true) | ||
{ | ||
throw new BadMethodCallException(sprintf('Calling "%s()" is not allowed.', __METHOD__)); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function registerBundles() | ||
{ | ||
throw new BadMethodCallException(sprintf('Calling "%s()" is not allowed.', __METHOD__)); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function registerContainerConfiguration(LoaderInterface $loader) | ||
{ | ||
throw new BadMethodCallException(sprintf('Calling "%s()" is not allowed.', __METHOD__)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's a lot of copy/paste, maybe a private function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, why not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looking at the code, it's actually done that way everywhere so I'm actually not so sure about that after all... IMO to be easier to maintain, you would be better off having a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering your last comment, I think I'll leave this like it is for now. |
||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function boot() | ||
{ | ||
throw new BadMethodCallException(sprintf('Calling "%s()" is not allowed.', __METHOD__)); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function shutdown() | ||
{ | ||
throw new BadMethodCallException(sprintf('Calling "%s()" is not allowed.', __METHOD__)); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getBundles() | ||
{ | ||
return array_map(function (BundleInterface $bundle) { | ||
return new BootingBundle($bundle); | ||
}, $this->innerKernel->getBundles()); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getBundle($name, $first = true) | ||
{ | ||
if (is_array($bundle = $this->innerKernel->getBundle($name, $first))) { | ||
return array_map(function (BundleInterface $bundle) { | ||
return new BootingBundle($bundle); | ||
}, $bundle); | ||
} | ||
|
||
return new BootingBundle($bundle); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function locateResource($name, $dir = null, $first = true) | ||
{ | ||
return $this->innerKernel->locateResource($name, $dir, $first); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getName() | ||
{ | ||
return $this->innerKernel->getName(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getEnvironment() | ||
{ | ||
return $this->innerKernel->getEnvironment(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function isDebug() | ||
{ | ||
return $this->innerKernel->isDebug(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getRootDir() | ||
{ | ||
return $this->innerKernel->getRootDir(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getContainer() | ||
{ | ||
throw new BadMethodCallException(sprintf('Calling "%s()" is not allowed.', __METHOD__)); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getStartTime() | ||
{ | ||
return $this->innerKernel->getStartTime(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getCacheDir() | ||
{ | ||
return $this->innerKernel->getCacheDir(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getLogDir() | ||
{ | ||
return $this->innerKernel->getLogDir(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getCharset() | ||
{ | ||
return $this->innerKernel->getCharset(); | ||
} | ||
} |
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.
Just as I did, you're missing this:
😅
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 😕
Uh oh!
There was an error while loading. Please reload this page.
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.
Ie. you'd inherit the wrong
@param
doc fromContainer::__construct
.@inheritdoc
is optional, however using it can be seen a best practice;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 😉