Skip to content

[DI] Simplify AutowirePass and other master-only cleanups #21797

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 1 commit into from
Feb 28, 2017

Conversation

nicolas-grekas
Copy link
Member

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

A few minor cleanups and fixes, and an overall simplification of AutowirePass.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

LGTM

if ($value instanceof ArgumentInterface) {
return $this->describeValue($value->getValues(), $omitTags, $showArguments);
}

if ($value instanceof Definition) {
Copy link
Member

Choose a reason for hiding this comment

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

changing the order here is because of the needs of #21708, right ? IMO, this is a clear sign that your other PR breaks BC (other bundles may have similar code)

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no BC issue: ArgumentInterface is a new feature of master

Copy link
Member

Choose a reason for hiding this comment

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

but existing code might check for Definition. And in your PRs, you have ServiceLocatorArgument extending Definition but which should not be processed like a Definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

The existing code won't get a new ServiceLocatorArgument all of a sudden, so if they do, someone did write some new code - thus we're out of BC concerns.
Processing ServiceLocatorArgument as a Definition wouldn't be a big issue btw. Existing code won't get the added semantic, but they will not break. Note that this is also true for pure ArgumentInterface instances also.

Copy link
Member

Choose a reason for hiding this comment

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

@nicolas-grekas existing bundles may have compiler passes processing the container. And Symfony 3.3 will change some existing services to use such argument.
Pure ArgumentInterface would be an argument for which they cannot do special processing. A Definition which must not be processed like a Definition is much worse, as they will think they can process it.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #21770

@fabpot
Copy link
Member

fabpot commented Feb 28, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 34e5cc7 into symfony:master Feb 28, 2017
fabpot added a commit that referenced this pull request Feb 28, 2017
…(nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Simplify AutowirePass and other master-only cleanups

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

A few minor cleanups and fixes, and an overall simplification of AutowirePass.

Commits
-------

34e5cc7 [DI] Simplify AutowirePass and other master-only cleanups
@nicolas-grekas nicolas-grekas deleted the di-autow-fix branch February 28, 2017 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants