Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
[Cache] Fix example for using cache tags with TagAwareCacheInterface#12181
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
OskarStark commentedAug 16, 2019
@Nyholm could you please verify this docs change? Thank you 😃 |
OskarStark left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Thanks@pavelmaca for the change and@Nyholm for the review 👍
Nyholm commentedAug 16, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh wait. I will review it shortly. |
Nyholm left a comment
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.
Hey,
Thank you for this PR. TheTagAwareCacheInterface is an interface for the cache adapter. What we get passed to our function is a cache item.
Nyholm commentedAug 16, 2019
👎 (to be clear) |
OskarStark commentedAug 16, 2019
@pavelmaca can you explain a bit why you did this change or which error do you have? |
Nyholm commentedAug 16, 2019
I did find a bug when testing this out. Maybe that was what@pavelmaca experienced? |
cache.rst Outdated
| use Symfony\Contracts\Cache\TagAwareCacheInterface; | ||
| $value0 = $pool->get('item_0', function (ItemInterface $item) { | ||
| $value0 = $pool->get('item_0', function (TagAwareCacheInterface $item) { |
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.
It is the$pool that should be aTagAwareCacheInterface.
@pavelmaca could you update this PR a little? Could you wrap this code into a function or a class and make sure$pool is aTagAwareCacheInterface?
Nyholm commentedAug 16, 2019
More info here:symfony/symfony#33201 |
pavelmaca commentedAug 16, 2019
@Nyholm Your findings are correct insymfony/symfony#33201 Updated doc with function and autowired $pool |
| $value0 = $pool->get('item_0', function (ItemInterface $item) { | ||
| $item->tag(['foo', 'bar']) | ||
| // using autowiring |
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.
| // using autowiring |
Nyholm left a comment
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.
Oscar, what do you think? Should we create a class and use constructor injection or is that overkill?
It would be more clear since injection to a function is generally only used with controllers.
| $value0 = $pool->get('item_0', function (ItemInterface $item) { | ||
| $item->tag(['foo', 'bar']) | ||
| // using autowiring | ||
| public function listProducts(TagAwareCacheInterface $pool) |
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.
Update$pool to$myCachePool to match the config below.
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.
Maybe use a more generic name thanlistProducts. Maybefoo?
OskarStark commentedAug 20, 2019
I think this would make sense in this case, otherwise this must be a tage with controller arguments tag 👍 👍 for a class + injection |
javiereguiluz commentedOct 1, 2019
I tried to merge this but I couldn't fix the conflicts ... so I created in#12398 a new pull request with these changes and all the other changes requested by Tobias.@pavelmaca I kept your original commit, so you'll get credit for this contribution. Thanks. |
…, javiereguiluz)This PR was merged into the 4.3 branch.Discussion----------[Cache] Improved the example about cache tagsThis continues the work started in#12181.Commits-------f9d438f Fixed code2651f92 [Cache] Improved the example about cache tagsce0c625 [Cache] Fix example for using cache tags with TagAwareCacheInterface
Using Cache Tags section has invalid example. Should use TagAwareCacheInterface.