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] Add missing deprecation when fetching private services from ContainerBuilder #25244

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
7 participants
@nicolas-grekas
Member

nicolas-grekas commented Dec 1, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25242
License MIT
Doc PR -
@stof

stof approved these changes Dec 1, 2017

@chalasr

chalasr approved these changes Dec 1, 2017

@fabpot

fabpot approved these changes Dec 1, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 1, 2017

Member

Tests are red though, and this seems related.

Member

fabpot commented Dec 1, 2017

Tests are red though, and this seems related.

@xabbuh

xabbuh approved these changes Dec 1, 2017

foreach ($this->definitions + $this->aliasDefinitions as $id => $definition) {
if (!$definition->isPublic() || $definition->isPrivate()) {
$this->removedIds[$id] = true;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

This is for ppl doing that, which is a no-op, but one we catch now:
image

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

This is for ppl doing that, which is a no-op, but one we catch now:
image

@@ -733,6 +733,7 @@ public function testEnvInId()
PsrContainerInterface::class => true,
ContainerInterface::class => true,
'baz_%env(BAR)%' => true,
'bar_%env(BAR)%' => true,

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

the added entries are the same as on 4.0/master: we now list the public-by-private legacy services as removed, even if they're not really for BC in 3.4.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

the added entries are the same as on 4.0/master: we now list the public-by-private legacy services as removed, even if they're not really for BC in 3.4.

This comment has been minimized.

@chalasr

chalasr Dec 1, 2017

Member

won't they appear as removed even if they are not in 4.0?

@chalasr

chalasr Dec 1, 2017

Member

won't they appear as removed even if they are not in 4.0?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

wdyt? they are in 4.0

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

wdyt? they are in 4.0

This comment has been minimized.

@chalasr

chalasr Dec 1, 2017

Member

definition isn't removed if injected twice or more, is it? If it isn't, wouldn't getRemovedIds() return an id that has not been removed? (I'm wondering why we populate removedIds from another place that the one which actually removes unused definitions)

@chalasr

chalasr Dec 1, 2017

Member

definition isn't removed if injected twice or more, is it? If it isn't, wouldn't getRemovedIds() return an id that has not been removed? (I'm wondering why we populate removedIds from another place that the one which actually removes unused definitions)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

Now green

Member

nicolas-grekas commented Dec 1, 2017

Now green

@stof

stof approved these changes Dec 1, 2017

@fabpot

fabpot approved these changes Dec 1, 2017

@xabbuh

xabbuh approved these changes Dec 1, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 1, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Dec 1, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 93c0b38 into symfony:3.4 Dec 1, 2017

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

bug #25244 [DI] Add missing deprecation when fetching private service…
…s from ContainerBuilder (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Add missing deprecation when fetching private services from ContainerBuilder

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

Commits
-------

93c0b38 [DI] Add missing deprecation when fetching private services from ContainerBuilder

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-deprec branch Dec 1, 2017

This was referenced Dec 4, 2017

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