Skip to content
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

Ensure Ephemeral driver always works with object clones #381

Closed
wants to merge 1 commit into from

Conversation

orcinus
Copy link

@orcinus orcinus commented Apr 22, 2018

Fix for #377

The Ephemeral driver methods Ephemeral::getData and Ephemeral::storeData use object references if data stored in cache is an object. This means that, e.g.

  1. storing object $foo in cache
  2. doing $foo->name = "not my name"
  3. and then reading from cache will produce an object with name set to "not my name"

Obviously, the data stored in cache should not be a reference, and should be invariant except when changed through the cache driver itself.

Simple clone if data is an object, as implemented here, should do the trick. This could've been written as a nested ternary, but that would've been ugly/unreadable.

@coveralls
Copy link

coveralls commented Apr 22, 2018

Coverage Status

Coverage decreased (-0.07%) to 94.987% when pulling 192488e on orcinus:orc-ephemeral-clone into a6f14f7 on tedious:master.

@orcinus
Copy link
Author

orcinus commented Apr 22, 2018

Forgot to mention - the change in Ephemeral::storeData isn't really necessary, assuming nothing inside the driver will ever change the data. But, thought it might be a good idea (defensive programming) to clone the object there as well. Feel free to remove.

@Ruud-Zuiderlicht
Copy link

Forgot to mention - the change in Ephemeral::storeData isn't really necessary, assuming nothing inside the driver will ever change the data. But, thought it might be a good idea (defensive programming) to clone the object there as well. Feel free to remove.

I think cloning at storeData is required to prevent 3rd party code from storing, then changing the object from outside of the Ephemeral driver.

In any case, what is the status of this change?

@mbaynton
Copy link
Contributor

I think that serialize() and deserialize() are better options than clone(), because that's what's used in the production drivers. The serialization/deserialization process is of course customizable, and if the Ephemeral driver does it the same way as the other drivers it allows me to write tests of my app that verify correct caching and serialization by just using the Ephemeral driver in the tests.

PHP's default clone also is a shallow copy so you're still going to have this issue with nested objects. This might also require developers to make their stuff clone properly in order to use it in tests when doing so isn't otherwise necessary.

@orcinus
Copy link
Author

orcinus commented Jan 14, 2019

Good point.

@mbaynton
Copy link
Contributor

As I'm coding this up, I actually like Ephemeral the way it is.

In production, I'm using the Composite driver to stack a limited-size Ephemeral layer on top of Redis. It's nice that those in-memory cache hits don't have serialization/deserialization overhead, and in the case of object graphs even more nice that the ephemerally cached objects retain objects they reference. Then I avoid making deep serialized copies of them (would be invalidation nightmare) in the more persistent cache by just customizing PHP's serialization behavior.

I'm trying out leaving Ephemeral as-is, but making a SerializingEphemeral extension as well for use in test.

@tedivm
Copy link
Member

tedivm commented Feb 21, 2022

I've rebuild the test suite to work on Github- please rebase against the main branch and reopen this pull request if you're still interested in having it merged. Thank you!

@tedivm tedivm closed this Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants