Skip to content

Fix deprecation for cache.app adapter with tags #58830

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

Closed
wants to merge 2 commits into from

Conversation

keulinho
Copy link
Contributor

@keulinho keulinho commented Nov 11, 2024

Q A
Branch? 7.2
Bug fix? yes
New feature? no
Deprecations? changed
Issues Fix #58790
License MIT

As discussed in the issue and looking at the code and the original issues here
#54339
and
twigphp/Twig#3869
I figured that the deprecation actually never did what it intended to do.

When I use the cache.app adapter in a new pool together with the tags option symfony creates a new tag aware pool, however cache.app itself won't be taggable. The only case I see how to make app.cache an instance of TagAware would be to declare that pool manually.

As detailed in the issue actually using cache.app.taggable as adapter won't even work.

@rpkamp & @alexandre-daubois maybe you could verify as you where involved in the original issues.

@keulinho
Copy link
Contributor Author

As a matter of fact when you declare a new pool with tags but without an explicit adapter it will fallback here to use app.cache as the adapter: https://github.com/symfony/symfony/blob/7.2/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L2404

This wouldn't throw the deprecation, but manually specifying that behaviour will throw the deprecation.

@OskarStark OskarStark changed the title Fix deprecation for cache.app adapter with tags Fix deprecation for cache.app adapter with tags Nov 11, 2024
@keulinho
Copy link
Contributor Author

Ok i'm really questioning what was the original goal here, as the tests show you can not configure the cache.app pool manually as that is reserved, I guess the whole deprecation can be removed and we are better off reverting the original PR

the only thing I imagine that could trigger the original issue then would be manually decorating `cache.app and I'm not sure if symfony should check this, or where to best check this, but it can not happen in the framework extension 🤔

@rpkamp
Copy link
Contributor

rpkamp commented Nov 12, 2024

We had set framework.cache.app set to use cache.adapter.redis_tag_aware, which Symfony allowed just fine, but that makes cache.app TagAware. Which is also fine, until you also start using Twig Caching, which wraps app.cache and make it tag aware, so it becomes tag aware "twice" and Twig caching doesn't work at all, but there is no indication anywhere that it doesn't work. It just MISSes all cache fetches, there are no exceptions, no warning, no nothing, it just doesn't work, and if you don't look close enough you'll never know.

So yes, there is a real use case here that this solves.

@keulinho
Copy link
Contributor Author

i guess i see, but that is not what was deprecated, because what was deprecated was using cache.app as adapter in any tagged pool, let me try to adjust the test case to your example

@keulinho
Copy link
Contributor Author

Changed the deprecation now that it checks for framework.cache.app and framework.cache.system as well, as both hard code tags = false so it makes no sense to configure them as tag aware right?

And now I understand also using cache.app.taggable as alternative, that makes only sense when using that service somewhere, but not on adapter level

@keulinho
Copy link
Contributor Author

Ok now apparently there are other test cases that expect that one can use cache.adapter.redis_tag_aware for cache.app 😮‍💨

@rpkamp
Copy link
Contributor

rpkamp commented Nov 12, 2024

Changed the deprecation now that it checks for framework.cache.app and framework.cache.system as well, as both hard code tags = false so it makes no sense to configure them as tag aware right?

Given the full context, indeed it does not. However, when you don't have the full context and are just configuring stuff in framework.yaml there is no way to know that app cache must not be taggable 🤷

@keulinho
Copy link
Contributor Author

Closing this as the behaviour in question is covered by test, instead I've now opted for removing the deprecation as that was quite clearly wrong.

see #58950

@keulinho keulinho closed this Nov 20, 2024
@symfony symfony deleted a comment from carsonbot Nov 20, 2024
nicolas-grekas added a commit that referenced this pull request Nov 20, 2024
…apter taggable" (keulinho)

This PR was merged into the 7.2 branch.

Discussion
----------

[FrameworkBundle] Revert " Deprecate making `cache.app` adapter taggable"

This reverts commit eed5b28.

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #58790
| License       | MIT

As discussed in former PR the deprecation was wrong, and trying to solve the deprecation will lead in errors.
Refer to #58830 for detailed analysis.

[according to the creator](#58830 (comment)) of the original issue the behaviour that is making problems is actually covered by tests, so I'm not sure how to proceed, but the deprecation is clearly wrong and should not make it to 7.2!

Commits
-------

25f0925 Revert "[FrameworkBundle] Deprecate making `cache.app` adapter taggable"
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Nov 20, 2024
…apter taggable" (keulinho)

This PR was merged into the 7.2 branch.

Discussion
----------

[FrameworkBundle] Revert " Deprecate making `cache.app` adapter taggable"

This reverts commit eed5b284618ec95eedbc376fb140b8fc24619372.

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony/symfony#58790
| License       | MIT

As discussed in former PR the deprecation was wrong, and trying to solve the deprecation will lead in errors.
Refer to symfony/symfony#58830 for detailed analysis.

[according to the creator](symfony/symfony#58830 (comment)) of the original issue the behaviour that is making problems is actually covered by tests, so I'm not sure how to proceed, but the deprecation is clearly wrong and should not make it to 7.2!

Commits
-------

25f0925d9d2 Revert "[FrameworkBundle] Deprecate making `cache.app` adapter taggable"
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.

Not possible to use cache.app.taggable adapter for cache pool
5 participants