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

Don't hash cache keys #91

Closed tremby closed 9 years ago

tremby commented 9 years ago

This reverts most of commit 9890bb7f08c573909d1c9d4d5e8dea512a5b677c, and its author @gsteel will need to update his Memcached CacheInterface implementation after this patch.

There is some discussion at https://github.com/prismicio/php-kit/issues/83

Before this patch, most cache keys were being hashed before being sent to the CacheInterface, to limit their length.

However, though some cache backends have low key length limits, many (including APC, which Prismic supports, and Redis and more) do not. For these, the hashing operation is not required. It is preferable when administrating and debugging a cache for the keys not to be hashed, since when hashed the keys are no longer sortable in an appreciable way and there is no clue to what the corresponding cache values might contain.

Notably, one item cached by Prismic (the API data) did not have its key hashed first anyway.

This patch also adds some documentation to CacheInterface to advise developers to check if the cache backend they are writing an implementation for has such a limit, and provides a suggestion as to what to do if so.

tremby commented 9 years ago

Will rebase on master so there's no conflict...

gsteel commented 9 years ago

FWIW, I think you've got a good point. Trying to fix a cache implementation detail from the SearchForm is just the wrong place for it - It should be the responsibility of whatever cache adapter you're using. The native memcache extension in php silently truncates keys to 250 characters so it's only a real problem, as it was in my case, when using adapters such as Zend\Cache which validate the key first and throw exceptions when it exceeds the limit.

Edit/Clarification - The native memcache extension will silently truncate your key, however, the newer memcached extension will fail to store data with a key length > 250, return false and leave an error message A BAD KEY WAS PROVIDED/CHARACTERS OUT OF RANGE

tremby commented 9 years ago

Thanks for that. Yeah, you could for example throw it through md5 if and only if it is longer than 250 characters. Obviously do it with a private method on your implementation so you're not duplicating code in each other method. (Then again, I believe you can't even list out the current keys from a running memcached, so maybe always hashing it is simpler in that particular case, since the unedited keys wouldn't be useful for debugging anyway!)

erwan commented 9 years ago

Alright, since gsteel is OK with that I'll merge the pull request.