-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
container:debug CLI output improvements for excluded services #48606
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
container:debug CLI output improvements for excluded services #48606
Conversation
c27c7fc
to
e92e0b5
Compare
Good catch. I'd suggest being more aggressive and just not list those services, and do it on 6.2. WDYT? |
@nicolas-grekas I think your proposal would make even more sense: no point in showing the service details for a class that is not, well, a service. 😄 Will update my PR for that 👍 |
e92e0b5
to
6e96730
Compare
@nicolas-grekas so I updated the implementation: now based on 6.2 and I just excluded the services from the choice list. However a probably better implementation would be to add a And if we go that route, should I just look up every definition in this call and check or is there a smarter way? |
6e96730
to
9d2f10f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as is, no need for more APIs.
Thank you @apfelbox. |
@nicolas-grekas the drawback of not adding a new API is that the call without a filter php bin/console debug:container will still list them. This call doesn't go through this method, but instead through the descriptors (and they call Should we ignore that? |
Normally I test whether the exclusion rules from the service container of my bundles work correctly by just using the
debug:container
command and looking whether my excluded services occur in the last.However, due to the latest changes in #46279 all services (excluded or not) are always in this list.
You need to open the definition to see if it was excluded:
This PR proposes two things:
(wording tbd)
If this looks like something you want to include, I will add the "graying out" part to the table view as well.
wdyt?