8000 [FrameworkBundle] Fix CacheCollectorPass priority by chalasr · Pull Request #23018 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Fix CacheCollectorPass priority #23018

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 1, 2017
Merged

[FrameworkBundle] Fix CacheCollectorPass priority #23018

merged 1 commit into from
Jun 1, 2017

Conversation

chalasr
Copy link
Member
@chalasr chalasr commented Jun 1, 2017
Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR #22960 (comment)

It was run before optimization, so child definitions were not resolved yet.

@chalasr chalasr mentioned this pull request Jun 1, 2017
Merged
@chalasr chalasr force-pushed the cache_collector_priority branch 2 times, most recently from 55b776c to 432072c Compare June 1, 2017 15:55
@tifabien
Copy link
Contributor
tifabien commented Jun 1, 2017

@chalasr I just tested your PR in my project and it seems it solves my issue :) Should I have a short explanation of what was the problem?

Thanks a lot!

@chalasr
Copy link
Member Author
chalasr commented Jun 1, 2017 •
edited
Loading

@tifabien The cache.app service extends the cache.adapter.filesystem one.
There is a compiler pass responsible for completing all services which extend another (merging from the parent). This pass processes the container in its optimization phase.
On the other hand, there is a compiler pass responsible for decorating all cache.pool tagged services to make them "traceable" and for injecting them into the data collector. Given this pass was processing the container before its optimization phase, child services weren't complete yet (hence the missing class). This makes it run after optimization, so the data collector pass can rely on the class even for services which inherit it from their parent.

You're welcome, thanks for confirming.

nicolas-grekas
nicolas-grekas reviewed Jun 1, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Resources/config/cache_debug.xml Outdated
@@ -7,8 +7,8 @@
<services>
<defaults public="false" />

<!-- DataCollector -->
<service id="data_collector.cache" class="Symfony\Component\Cache\DataCollector\CacheDataCollector">
<!-- DataCollector (must be public to prevent inlining) -->
Copy link
Member

Choose a reason for hiding this comment

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

public to prevent inlining, made private in Cache...Pass

@chalasr chalasr force-pushed the cache_collector_priority branch from 432072c to a347ae3 Compare June 1, 2017 17:00
@chalasr chalasr force-pushed the cache_collector_priority branch from a347ae3 to 28b253a Compare June 1, 2017 17:06
nicolas-grekas
nicolas-grekas approved these changes Jun 1, 2017
View reviewed changes
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.

👍

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Jun 1, 2017
@fabpot
Copy link
Member
fabpot commented Jun 1, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 28b253a into symfony:3.3 Jun 1, 2017
fabpot added a commit that referenced this pull request Jun 1, 2017
@fabpot
bug #23018 [FrameworkBundle] Fix CacheCollectorPass priority (chalasr)
156a50d 
This PR was merged into the 3.3 branch.

Discussion
----------

[FrameworkBundle] Fix CacheCollectorPass priority

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | #22960 (comment)

It was run before optimization, so child definitions were not resolved yet.

Commits
-------

28b253a Fix CacheCollectorPass priority
@chalasr chalasr deleted the cache_collector_priority branch June 1, 2017 18:39
@fabpot fabpot mentioned this pull request Jun 5, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
5 participants
@chalasr @tifabien @fabpot @nicolas-grekas @carsonbot
0