Skip to content

Removed deprecations in DependencyInjection component#14155

Closed
dosten wants to merge 10 commits into
symfony:masterfrom
dosten:dependency-injection-deprecations
Closed

Removed deprecations in DependencyInjection component#14155
dosten wants to merge 10 commits into
symfony:masterfrom
dosten:dependency-injection-deprecations

Conversation

@dosten

@dosten dosten commented Apr 1, 2015

Copy link
Copy Markdown
Contributor
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

@dosten

dosten commented Apr 1, 2015

Copy link
Copy Markdown
Contributor Author

The tests didn't run, @fabpot can you run them manually?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The $synchronizers variable can also be removed as its not really used anymore now.

On line 666 ( 😈 ) and 677

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

@sstok

sstok commented Apr 1, 2015

Copy link
Copy Markdown
Contributor

LGTM 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be removed entirely instead. The case of factories is already covered by the previous condition

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@stof

stof commented Apr 1, 2015

Copy link
Copy Markdown
Member

this is likely to conflict with #14127

@fabpot

fabpot commented May 16, 2015

Copy link
Copy Markdown
Member

Thank you @dosten.

@fabpot fabpot closed this May 16, 2015
fabpot added a commit that referenced this pull request May 16, 2015
…(dosten)

This PR was squashed before being merged into the 3.0-dev branch (closes #14155).

Discussion
----------

Removed deprecations in DependencyInjection component

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

Commits
-------

76ff5a9 Removed deprecations in DependencyInjection component
@dosten dosten deleted the dependency-injection-deprecations branch May 16, 2015 14:28

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this and also the special logic in has looks wrong to me. the container should not know anything about the service_container anymore and just treat it like any other service.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nvm, it's purpose is to not be able to overwrite the special service which is ok

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, do we actually need this kind of special service at all or could we get rid of it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xabbuh I think we need it to inject the entire container into a service (not recommended I know but a lot of developers do)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hacfi That's true.

@hacfi

hacfi commented May 19, 2015

Copy link
Copy Markdown
Contributor

@dosten very nice work!

fabpot added a commit that referenced this pull request Jul 4, 2015
This PR was merged into the 3.0-dev branch.

Discussion
----------

[DependencyInjection] Remove deprecated code

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

This code has been removed in #14155 but seems that a wrong merge didn't removed it.

Commits
-------

f2fef91 Removed deprecated code
@fabpot fabpot mentioned this pull request Nov 16, 2015
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.

8 participants