Skip to content

[Cache] Handle serialization failures for Memcached #23615

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
Jul 26, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jul 21, 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 -

Fixes two issues with serialization + memcached: with the memcached extension, the default serializer is automatically selected as igbinary when possible, native php otherwise. That creates obvious migration/portability issues (ie just installing igbinary wipes out the value of your cache.)

Then, handling unserializing failures (esp. "php_incomplete_class") is a paramount feature of the component. You must be able to deal with migrating you code base without being blocked by some legacy serialized data.

@nicolas-grekas
Copy link
Member Author

So, what would be your stand on #19895?

@robfrawley
Copy link
Contributor

robfrawley commented Jul 21, 2017

Accidentally hit delete when editing my original message; here it is again:

Does this make the php serializer default now?! Do we really need to force this default for users, especially to an inferior serialization method? If people want portability and the environment's they deploy to differ, they can already explicitly set whatever serializer they want, right?

I think my initial reaction was an uneducated gut-reaction without fully understanding the intentions of this PR; if your goal is #19895 (comment), then I'm onboard, and may even have some time to contribute towards those goals.

@nicolas-grekas
Copy link
Member Author

My goal with this PR is to have a predictable, thus safe, default behavior. :)

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I would merge this as a bug fix in 3.3 instead. Wiping the cache should never be an issue (should probably be done anyway on a new deploy).

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.3 July 22, 2017 13:19
@nicolas-grekas nicolas-grekas modified the milestones: 3.3, 3.4 Jul 22, 2017
@nicolas-grekas
Copy link
Member Author

Rebased to target 3.3

@nicolas-grekas nicolas-grekas merged commit cccc88f into symfony:3.3 Jul 26, 2017
nicolas-grekas added a commit that referenced this pull request Jul 26, 2017
…as-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[Cache] Handle serialization failures for Memcached

| 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        | -

Fixes two issues with serialization + memcached: with the memcached extension, the default serializer is automatically selected as igbinary when possible, native php otherwise. That creates obvious migration/portability issues (ie just installing igbinary wipes out the value of your cache.)

Then, handling unserializing failures (esp. "php_incomplete_class") is a paramount feature of the component. You must be able to deal with migrating you code base without being blocked by some legacy serialized data.

Commits
-------

cccc88f [Cache] Handle unserialization failures for Memcached
@nicolas-grekas nicolas-grekas deleted the cache-ser branch July 26, 2017 06:22
@fabpot fabpot mentioned this pull request Aug 1, 2017
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