Skip to content

[FrameworkBundle] grab a service from the container only if it exists #53572

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

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jan 18, 2024

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #53551
License MIT

@xabbuh
Copy link
Member Author

xabbuh commented Jan 18, 2024

The root cause for the observed behaviour is that the list of pool names passed to the constructor of CachePoolClearCommand command contains also the ids of cache pools that are later cleaned up by the RemoveUnusedDefinitionsPass. An alternative solution would be to collect available cache pool names at a later stage during compilation when the cleanup of unused service definitions has already happened.

@nicolas-grekas
Copy link
Member

Can't we check if the service exists before accessing it? Because this strategy is going to keep around pools that are not used, preventing them to be cleaned up by the compilation process.

@xabbuh xabbuh changed the title [Validator] keep a reference to all cache pools [Cache] keep a reference to all cache pools Jan 22, 2024
@xabbuh xabbuh changed the title [Cache] keep a reference to all cache pools [FrameworkBundle] grab a service from the container only if it exists Jan 22, 2024
@xabbuh
Copy link
Member Author

xabbuh commented Jan 22, 2024

indeed, that's a better idea

@xabbuh xabbuh changed the base branch from 5.4 to 6.3 January 22, 2024 10:26
@xabbuh xabbuh modified the milestones: 5.4, 6.3 Jan 22, 2024
@carsonbot carsonbot changed the title [FrameworkBundle] grab a service from the container only if it exists [Validator]  grab a service from the container only if it exists Jan 22, 2024
@OskarStark
Copy link
Contributor

Why the Validator label?

@xabbuh xabbuh changed the title [Validator]  grab a service from the container only if it exists [FrameworkBundle] grab a service from the container only if it exists Jan 22, 2024
@xabbuh
Copy link
Member Author

xabbuh commented Jan 22, 2024

probably because I made a typo in the initial title and didn't remove the label when fixing that so Carson re-adds it again

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one minor tweak

@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 8d8c8fe into symfony:6.3 Jan 22, 2024
@xabbuh xabbuh deleted the issue-53551 branch January 22, 2024 21:11
This was referenced Jan 30, 2024
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