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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public function process(ContainerBuilder $container)
}
$config = $container->getParameterBag()->resolveValue($config);

$tmpContainer = new ContainerBuilder($container->getParameterBag());
$tmpContainer = new ContainerBuilder($container->getParameterBag(), $container->getContext());
$tmpContainer->setResourceTracking($container->isTrackingResources());
$tmpContainer->addObjectResource($extension);

Expand Down
24 changes: 23 additions & 1 deletion src/Symfony/Component/DependencyInjection/ContainerBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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[]
Expand Down Expand Up @@ -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
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 😉

*/
public function __construct(ParameterBagInterface $parameterBag = null, Context $context = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires a new docblock.

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 as it won't bring anything. Appart if we need to add a description.

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.

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 Container::__construct (|null that is).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
*
Expand Down Expand Up @@ -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.
*
Expand Down
76 changes: 76 additions & 0 deletions src/Symfony/Component/DependencyInjection/Context.php
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont get me wrong ;-) but consider extending RuntimeException. $key is a valid argument by design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I've reflected what is done in the ParameterNotFoundException ^^

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 LogicException (which \InvalidArgumentException extends). The behavior won't change at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}
}
191 changes: 191 additions & 0 deletions src/Symfony/Component/HttpKernel/BootingKernel.php
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
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 $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__));
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a lot of copy/paste, maybe a private function throwBadMethodCallException(string $method) would be appropriate here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, why not.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 BadMethodCallExceptionFactory::create(string $method) which could be reused anywhere

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.

Considering your last comment, I think I'll leave this like it is for now.
The benefits of creating & using a new method, an exception factory, or a dedicated exception are too low to be considered over a simple search & replace.

}

/**
* {@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();
}
}
Loading
0