Skip to content

[DependencyInjection] The class of a service definition might be passed as parameter #32279

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[DependencyInjection] The class of a service definition might be passed as parameter #32279

wants to merge 1 commit into from

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Jun 29, 2019 •
edited
Loading

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

While working on #32266, I discovered that we can pass an instance of Parameter as $class to a Definition, although the docblock says string|null.

symfony/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

Line 931 in 6eeec65

$container->setDefinition('foo', new Definition(new Parameter('class')));

I'd like to adjust the docblock, so nobody will ever be tempted to add a type-hint to setClass() again. 😉

I've also added a test because the test failure that I got wasn't really obviously pointing at the problem.

@derrabus derrabus force-pushed the bugfix/class-can-be-parameter branch from 2ebd755 to af42a0f Compare June 29, 2019 16:13
@derrabus derrabus changed the title The class of a service definition might be passed as parameter. [DependencyInjection] The class of a service definition might be passed as parameter Jun 29, 2019
@xabbuh xabbuh added this to the 3.4 milestone Jun 30, 2019
@xabbuh
Copy link
Member

xabbuh commented Jun 30, 2019

I discovered that we can pass an instance of Parameter as $class to a Definition, although the docblock says string|null

This looks wrong to me. The usage should be new Defintion('%class%') IMO.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 30, 2019 •
edited
Loading

The usage should be new Defintion('%class%') IMO.

That is not the same semantics: Parameter objects are compiled to a call to getParameter(), while %foo% references are resolved when compiling.

@nicolas-grekas
Copy link
Member

I'm wondering if we shouldn't reject this PR, and deprecate passing a Parameter instead. This looks like something that is not needed to me.

@derrabus
Copy link
Member Author

derrabus commented Jul 4, 2019

I'm wondering if we shouldn't reject this PR, and deprecate passing a Parameter instead. This looks like something that is not needed to me.

That would certainly improve the maintainability of the class. Shall I open a PR?

@nicolas-grekas
Copy link
Member

yes please!

@fabpot
Copy link
Member

fabpot commented Jul 5, 2019

Closing as I agree that this "use case" should be deprecated instead.

@fabpot fabpot closed this Jul 5, 2019
@derrabus derrabus mentioned this pull request Jul 5, 2019
Merged
@derrabus derrabus deleted the bugfix/class-can-be-parameter branch July 5, 2019 16:19
@derrabus
Copy link
Member Author

derrabus commented Jul 5, 2019

New PR: #32390

Tobion added a commit that referenced this pull request Jul 5, 2019
@Tobion
feature #32390 [DependencyInjection] Deprecated passing Parameter ins…
43c14dd 
…tances as class name to Definition (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Deprecated passing Parameter instances as class name to Definition

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

This PR deprecates the undocumented possibility to use a `Parameter` instance instead of a string as class name for a `Definition`. This was discovered while working on #32266.

Commits
-------

edfc9d6 Deprecated passing Parameter instances as class name to Definition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@derrabus @xabbuh @nicolas-grekas @fabpot @carsonbot