search

prismicio-community / php-kit

Community maintained development kit for Prismic and the PHP language
https://prismic.io
Other
108 stars 82 forks source link

Unserialize cached api data only if the cache returns a string. #134

Closed gsteel closed 6 years ago

gsteel commented 7 years ago

This seems like a safe thing to do. If you’re using a cache implementation that automatically unserializes data as it’s retrieved from the cache Api::get() will be trying to unserialize an object. This change allows for a wider range of cache implementations without breaking existing behaviour.

srenault commented 7 years ago

Ok I'll look at it!

srenault commented 7 years ago

What's weird is we serialize (https://github.com/prismicio/php-kit/blob/master/src/Prismic/Api.php#L341) and unserialize whatever the cache implementation we use. Can you give the name of one cache implementation that unserializes automatically data?

gsteel commented 7 years ago

I personally use a facade in front of Zend\Cache - Zend\Cache has an automatic serializer plugin. If I'm re-using the same cache implementation throughout an app, and this plugin is active, it causes problems with the Prismic SDK as data is unserialised coming out of the cache and again in the SDK.

It would probably be better if the SDK type hinted against PSR-6 Cache, or PSR-16 but that's a whole different ball game I guess? Would it be worth while to put together a pull request for this?

I figured that in this case, the Prismic\Cache interface does not specify a return value explicitly for get() so checking for a string before unserialising is probably no bad thing anyhow.

srenault commented 7 years ago

I need to perform some tests before merging this PR. These days, I don't have much time to do it.

srenault commented 7 years ago

Maybe the best is to give control over serialize/unserialize stuff. We could add both methods in the CacheInterface. What do you think?

gsteel commented 7 years ago

IMO, get() needs an instance of ApiData, checking to see whether the cache returns a string to be unserialised, or a ready to go instance is a reasonable test - theoretically, you have no idea what kind of cache might be in use or whether that cache is going to return something you can actually use so the only way of making sure you get ApiData back from the cache would be to drop support for PHP 5 and add new methods to CacheInterface that have a return type hint, say Cache::getApiData() : ApiData; which would mean breaking changes and implementing more methods in whatever you've swapped out the default cache for. So, I guess I'm saying that I don't think increasing the complexity of the CacheInterface is a good idea.

As I mentioned previously, the kit would be more interoperable if it consumed a PSR\CacheInterface but in lieu of that, this PR does cover the (admittedly) edge case that the underlying cache automatically unserialises data on retrieval in a non-breaking way.

srenault commented 7 years ago

I think breaking compatibility isn't a big deal. Versioning can handle this. As you mentioned above, I'd prefer make the cache PSR-16 compliant instead of trying to fix the current code.