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] Throw useful error msg when trying to access not-found but private service #24444

Closed
nicolas-grekas opened this Issue Oct 5, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Oct 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 5, 2017

Member

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)

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 5, 2017

Member

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.)

Member

nicolas-grekas commented Oct 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 6, 2017

Member

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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 6, 2017

Member

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.

Member

nicolas-grekas commented Oct 6, 2017

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.

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 6, 2017

Member

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.)".

Member

sroze commented Oct 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Oct 6, 2017

Member

@sroze You probably missed #23741

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

feature #24484 [DI] Throw accurate failures when accessing removed se…
…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 Oct 10, 2017

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