Skip to content

[Cache] Allow decorating each cache.pool as debug adapters #42241

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 7 commits into from

Conversation

rvanlaak
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #41685
License MIT
Doc PR symfony/symfony-docs#...
  • introduces DebugAdapter to throw exception on failing save
  • adds cache.exception_on_save as parameter to configure the feature

@nicolas-grekas could you provide some initial feedback on whether this implementation is in the right direction?

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@rvanlaak rvanlaak marked this pull request as ready for review July 23, 2021 13:50
@carsonbot carsonbot added Status: Needs Review Cache Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jul 23, 2021
@carsonbot carsonbot changed the title [RFC][Cache] Allow decorating each cache.pool as debug adapters [Cache] Allow decorating each cache.pool as debug adapters Jul 23, 2021
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 23, 2021

That's an interesting approach, but to be honest I had another one in mind:
given that all errors end up being logged in CacheItem::log(), we could inject a throwing logger in debug mode instead.

That would give better DX, by giving proper error messages.

@nicolas-grekas
Copy link
Member

Closing in favor of #43148
Thanks for submitting!

nicolas-grekas added a commit that referenced this pull request Sep 24, 2021
…ion fails (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[Cache] Throw ValueError in debug mode when serialization fails

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | -
| Tickets       | Fix #41685
| License       | MIT
| Doc PR        | -

This feature allows spotting mistakes at the dev stage when trying to store unserializable objects (typically a closure) into a cache.

This PR replaces #42241 with a simpler and more focused approach: here we fail via a simple `ValueError`, which won't get caught by any try/catch into the Cache component, and we fail only when serialization fails - not on any failures of cache pool methods.

Commits
-------

0d3ede7 [Cache] Throw ValueError in debug mode when serialization fails
@rvanlaak rvanlaak deleted the cache-debug-exceptions branch September 25, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cache Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In debug mode, the cache component could throw when save() fails
3 participants