8000 [DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters as-a-service by nicolas-grekas · Pull Request #25288 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters as-a-service #25288

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

Merged
merged 2 commits into from
Dec 8, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
[DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters …
…as-a-service
  • Loading branch information
nicolas-grekas committed Dec 4, 2017
commit 0e18d3ec2b7dcc1c47658c4b18da8fca6042363b
3 changes: 2 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ CHANGELOG
4.1.0
-----

* allowed to pass an optional `LoggerInterface $logger` instance to the `Router`
* Allowed to pass an optional `LoggerInterface $logger` instance to the `Router`
* Added a new `parameter_bag` service with related autowiring aliases to acces parameters as-a-service
Copy link
Member

Choose a reason for hiding this comment

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

access

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix directly


4.0.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Component\DependencyInjection\ParameterBag\ContainerBagInterface;
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\DependencyInjection\ServiceSubscriberInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
Expand Down Expand Up @@ -110,6 +112,12 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('services.xml');
$loader->load('fragment_renderer.xml');

if (!interface_exists(ContainerBagInterface::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

why not just bumping the min version of the DI component instead ? This is a hard requirement already.

Copy link
Member Author
@nicolas-grekas nicolas-grekas Dec 8, 2017

Choose a reason for hiding this comment

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

because I don't want to be the one breaking the 3-cross-4 compat :)

$container->removeDefinition('parameter_bag');
$container->removeAlias(ContainerBagInterface::class);
$container->removeAlias(ParameterBagInterface::class);
}

if (class_exists(Application::class)) {
$loader->load('console.xml');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
<services>
<defaults public="false" />

<service id="parameter_bag" class="Symfony\Component\DependencyInjection\ParameterBag\ContainerBag">
<argument type="service" id="service_container" />
</service>
<service id="Symfony\Component\DependencyInjection\ParameterBag\ContainerBagInterface" alias="parameter_bag" />
<service id="Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface" alias="parameter_bag" />

<service id="event_dispatcher" class="Symfony\Component\EventDispatcher\EventDispatcher" public="true">
<tag name="container.hot_path" />
</service>
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ CHANGELOG
-----

* added support for variadics in named arguments
* added PSR-11 `ContainerBagInterface` and its `ContainerBag` implementation to access parameters as-a-service

4.0.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@

namespace Symfony\Component\DependencyInjection\Exception;

use Psr\Container\NotFoundExceptionInterface;

/**
* This exception is thrown when a non-existent parameter is used.
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class ParameterNotFoundException extends InvalidArgumentException
class ParameterNotFoundException extends InvalidArgumentException implements NotFoundExceptionInterface
{
private $key;
private $sourceId;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?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\ParameterBag;

use Symfony\Component\DependencyInjection\Container;

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
class ContainerBag extends FrozenParameterBag implements ContainerBagInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be made final, I see no reason to extend this rather than implementing the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't make classes final "by default", that's not the framework's policy.

Copy link
Contributor
@TomasVotruba TomasVotruba Dec 9, 2017

Choose a reason for hiding this comment

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

I think it's good idea. Could that F438 change for new classes?
Like adding scalar type is?

{
private $container;

public function __construct(Container $container)
{
$this->container = $container;
}

/**
* {@inheritdoc}
*/
public function all()
{
return $this->container->getParameterBag()->all();
}

/**
* {@inheritdoc}
*/
public function get($name)
{
return $this->container->getParameter($name);
}

/**
* {@inheritdoc}
*/
public function has($name)
{
return $this->container->hasParameter($name);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?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\ParameterBag;

use Psr\Container\ContainerInterface;
use Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException;

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
interface ContainerBagInterface extends ContainerInterface
Copy link
Contributor
@ro0NL ro0NL Dec 3, 2017

Choose a reason for hiding this comment

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

should we move this one namespace up? is typehinting SF\ContainerBagInterface instead of SF\ParamaterBagInterface really worth it? What about a ContainerParameterBag implem to enable typehinting ParamaterBagInterface?

in general #24738 needs ParamaterBagInterface::resolveValue() actually

Copy link
Member Author

Choose a reason for hiding this comment

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

should we move this one namespace up?

that's a parameter bag, should be in the namespace to me

is typehinting SF\ContainerBagInterface instead of SF\ParamaterBagInterface really worth it?

yes: the new interface is more tailored than, the existing one, better for autosuggestion by IDEs

What about a ContainerParameterBag implem to enable typehinting ParamaterBagInterface?

isn't that already what the new ContainerBag provides?

Copy link
Contributor

Choose a reason for hiding this comment

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

the new interface is more tailored than, the existing one, better for autosuggestion by IDEs

Yeah but why favor ContainerBagInterface over ParamaterBagInterface from the same package/namespace? Is it to provide 2 different PSR containers? Are we "hacking" autowiring?

class ContainerParameterBag implements ParamBagInterface, PSR\ContainerInterface
{ }

would do?

Copy link
Member Author

Choose a reason for hiding this comment

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

let's say you have ContainerBagInterface $bag type-hint.
typing $bag-> the IDE will suggest you with "get", "has" and "all".
if we do what you suggest, the IDE will also suggest all the other methods inherited from ParamaterBagInterface, which, if used, will throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I was thinking you'd prefer ContainerInterface $bag actually, which troubles autowiring when you need $container also. So we add this interface.. for DX. That's OK i guess :-)

Copy link
Contributor
@ro0NL ro0NL Dec 3, 2017

Choose a reason for hiding this comment

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

ContainerBagInterface::resolveValue would be useful? never mind, you'd typehint ParamaterBagInterface then i guess.

Copy link
Member Author
@nicolas-grekas nicolas-grekas Dec 3, 2017

Choose a reason for hiding this comment

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

ContainerBagInterface::resolveValue would be useful?

not for this PR
I changed my mind during the night, here it is, trivial to add :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait... do the point of this interface is just for DX? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there anything we do in a framework that is not for DX?
Anyway, "for DX" is not a technical thing. Here is the technical outcome: the interface is to declare a contract you want to adhere to: you explicitly state that you're going to need read-only access to the bag. That's the purpose of the interface.

{
/**
* Gets the service container parameters.
*
* @return array An array of parameters
*/
public function all();
Copy link
Contributor

Choose a reason for hiding this comment

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

return type declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not possible: would make the interface incompatible with ParameterBagInterface.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm too late but, is this true?
https://3v4l.org/NPigR

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it isn't.


/**
* Replaces parameter placeholders (%name%) by their values.
*
* @param mixed $value A value
*
* @throws ParameterNotFoundException if a placeholder references a parameter that does not exist
*/
public function resolveValue($value);

/**
* Escape parameter placeholders %.
*
* @param mixed $value
*
* @return mixed
*/
public function escapeValue($value);

/**
* Unescape parameter placeholders %.
*
* @param mixed $value
*
* @return mixed
*/
public function unescapeValue($value);
}
0