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

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

Merged
merged 2 commits into from Dec 8, 2017

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Dec 3, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17160
License MIT
Doc PR -

There is one thing that prevents us from not injecting the container: access to the parameter bag.
This PR fixes this limitation by providing a PSR-11 ContainerBagInterface + related implementation, and wiring it as a service that ppl can then also autowire using the new interface as a type hint, or ParameterBagInterface.

Needed to complete e.g. #24738

/**
* @author Nicolas Grekas <p@tchwork.com>
*/
interface ContainerBagInterface extends ContainerInterface

This comment has been minimized.

@ro0NL

ro0NL Dec 3, 2017

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 3, 2017

Member

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?

This comment has been minimized.

@ro0NL

ro0NL Dec 3, 2017

Contributor

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?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 3, 2017

Member

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.

This comment has been minimized.

@ro0NL

ro0NL Dec 3, 2017

Contributor

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

This comment has been minimized.

@ro0NL

ro0NL Dec 3, 2017

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 3, 2017

Member

ContainerBagInterface::resolveValue would be useful?

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

This comment has been minimized.

@sroze

sroze Dec 4, 2017

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

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.

@ro0NL

ro0NL approved these changes Dec 3, 2017

@@ -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)) {
$container->removeDefinition('parameter_bag');
$container->removeAlias(ContainerBagInterface);

This comment has been minimized.

@chalasr

chalasr Dec 3, 2017

Member

missing ::class, same below

This comment has been minimized.

@ro0NL

ro0NL Dec 3, 2017

Contributor

wow :-)

This comment has been minimized.

@nicolas-grekas
@@ -5,6 +5,7 @@ CHANGELOG
-----
* added support for variadics in named arguments
* added PSR-11 `ContainerBagInterface` and its `ContainerBag` implementation to access parameters as-as-service

This comment has been minimized.

@chalasr

chalasr Dec 3, 2017

Member

typo as-a

*
* @return array An array of parameters
*/
public function all();

This comment has been minimized.

@Tobion

Tobion Dec 4, 2017

Member

return type declaration?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

Not possible: would make the interface incompatible with ParameterBagInterface.

This comment has been minimized.

@ciaranmcnulty

ciaranmcnulty May 31, 2018

Contributor

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

This comment has been minimized.

@sroze

sroze May 31, 2018

Member

Looks like it isn't.

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

This comment has been minimized.

@iltar

iltar Dec 4, 2017

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

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

This comment has been minimized.

@TomasVotruba

TomasVotruba Dec 9, 2017

Contributor

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

@ogizanagi

But this will probably lead to developers using this for use-cases for which we'll rather prone regular scalar values injection and local binding (#23718).

@chalasr

chalasr approved these changes Dec 4, 2017

👍 not fond of scalar injection :)

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 4, 2017

Should we make AbstractController use it?

public function setUp()
{
$this->containerBag = new ContainerBag(new Container($this->parameterBag = new ParameterBag(array(

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

on two lines would be more readable

This comment has been minimized.

@sroze

sroze Dec 4, 2017

Member

Oh. I love that you are saying that, let me change this right now 😛

public function testGetAllParameters()
{
$this->assertEquals(array(

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

on one line is OK here to me

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 4, 2017

Should we make AbstractController use it?

could be, but not in this PR

@nicolas-grekas nicolas-grekas added the Ready label Dec 7, 2017

@xabbuh

xabbuh approved these changes Dec 8, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 8, 2017

Thank you @sroze.

@nicolas-grekas nicolas-grekas merged commit 561cd7e into symfony:master Dec 8, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 8, 2017

feature #25288 [DI][FrameworkBundle] Add PSR-11 "ContainerBag" to acc…
…ess parameters as-a-service (nicolas-grekas, sroze)

This PR was merged into the 4.1-dev branch.

Discussion
----------

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

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17160
| License       | MIT
| Doc PR        | -

There is one thing that prevents us from not injecting the container: access to the parameter bag.
This PR fixes this limitation by providing a PSR-11 `ContainerBagInterface` + related implementation, and wiring it as a service that ppl can then also autowire using the new interface as a type hint, or `ParameterBagInterface`.

Needed to complete e.g. #24738

Commits
-------

561cd7e Add tests on the ContainerBag
0e18d3e [DI][FrameworkBundle] Add PSR-11 "ContainerBag" to access parameters as-a-service

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-container-bag branch Dec 8, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 8, 2017

Should we make AbstractController use it?

@chalasr unlocked if you want to make it happen :)

@@ -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

This comment has been minimized.

@stof

stof Dec 8, 2017

Member

access

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 8, 2017

Member

will fix directly

@@ -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)) {

This comment has been minimized.

@stof

stof Dec 8, 2017

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 8, 2017

Member

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

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Dec 11, 2017

feature #25439 Add ControllerTrait::getParameter() (chalasr)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Add ControllerTrait::getParameter()

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#25288 (comment)
| License       | MIT
| Doc PR        | n/a

Commits
-------

28397e5b2c Add ControllerTrait::getParameter()

fabpot added a commit that referenced this pull request Dec 11, 2017

feature #25439 Add ControllerTrait::getParameter() (chalasr)
This PR was merged into the 4.1-dev branch.

Discussion
----------

Add ControllerTrait::getParameter()

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25288 (comment)
| License       | MIT
| Doc PR        | n/a

Commits
-------

28397e5 Add ControllerTrait::getParameter()

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment