8000 [DI] Throw useful error msg when trying to access not-found but private service · Issue #24444 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[DI] Throw useful error msg when trying to access not-found but private service #24444

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

Closed
nicolas-grekas opened this issue Oct 5, 2017 · 6 comments
Assignees
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@nicolas-grekas
Copy link
Member
Q A
Bug report? no
Feature request? no
BC Break report? no
RFC? yes
Symfony version 3.4

Right now, when a service is not found, we throw a ServiceNotFoundException with some suggestions.
But when the lookup is for a private service that ended up being removed or inlined, that message is not accurate: we should instead hint about the service being private + suggest to either turn it public or not use the container at all (similar to the deprecation in Container::get()).

On 4.0, where services will be private by default, this looks like a DX requirement to me, so we should do it during the feat. freeze.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 5, 2017
@nicolas-grekas nicolas-grekas self-assigned this Oct 5, 2017
@stof
Copy link
Member
stof commented Oct 5, 2017

However, this would force us to keep a list of private ids in the dumped container (including removed/inlined services, that we don't even have today at all). What about the memory usage here (although it should be possible to keep this array in shared memory if being careful)

@nicolas-grekas
Copy link
Member Author

Correct. I think that it wouldn't matter if we put that list in a separate file (at least when using the multi-files dumped container.)

@sroze
Copy link
Contributor
sroze commented Oct 6, 2017

On 4.0, where services will be private by default

So is this the final decision of the core team? Either way, I also think it's a DX requirement.

@nicolas-grekas What do you mean by a separate file? We already have the privates within the container, can't we use this?

@nicolas-grekas
Copy link
Member Author

That's the current state yes, so it should stay: this is just the final logical step of the work put by the community in this area

About the list of private services, yes we can put it in the class for sure. The discussion was about finding a way to not put it there, in order to not bloat the container.

@xabbuh xabbuh added DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Oct 6, 2017
@sroze
Copy link
Contributor
sroze commented Oct 6, 2017 &# 8000 8226;

That's fair enough, let's go this way. We need to write some great documentation/articles about how this is important then I guess.

Sure, I get that it would be nice to move it somewhere else for performance reasons but for now I guess we can just re-use these privates for this specific issue :) Also, don't get what you mean by "we put that list in a separate file (at least when using the multi-files dumped container.)".

@chalasr
Copy link
Member
chalasr commented Oct 6, 2017

@sroze You probably missed #23741

@nicolas-grekas nicolas-grekas removed this from the 3.4 milestone Oct 8, 2017
fabpot added a commit that referenced this issue 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
@fabpot fabpot closed this as completed Oct 10, 2017
Sign up for free to jo 5043 in this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

6 participants
0