Skip to content

Conversation

@nicolas-grekas
Copy link
Member

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

See linked issue.
This will throw a useful message when accessing a removed service.
When setting a removed service, a deprecation notice will be thrown also so that in master we can throw an exception then.

@sroze
Copy link
Contributor

sroze commented Oct 8, 2017

Shouldn't we keep these synthetic and removed IDs in the container only for debug environments? Because here the value is only DX and the container is in theory taking more memory than before.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Oct 8, 2017

Introducing a different behavior between debug and non-debug is something that could led to subtle bugs, so should be prevented as much as possible.
Fortunately, the reason why you asked for it does not exist in practice: removed ids are split in a separate file that is loaded on demand, and the number of synthetic ids is very small.

@fabpot
Copy link
Member

fabpot commented Oct 10, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit fe7f26d into symfony:3.4 Oct 10, 2017
fabpot added a commit that referenced this pull request Oct 10, 2017
…rvices (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Throw accurate failures when accessing removed services

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

See linked issue.
This will throw a useful message when accessing a removed service.
When setting a removed service, a deprecation notice will be thrown also so that in master we can throw an exception then.

Commits
-------

fe7f26d [DI] Throw accurate failures when accessing removed services
@nicolas-grekas nicolas-grekas deleted the collect-privates branch October 10, 2017 07:42
This was referenced Oct 18, 2017
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.

4 participants