#11144: adding cache invalidation for classification store KeyConfig #11254
Conversation
|
@awroblewski you are right, I think it also would make sense to do the "named" based caching as well for |
… local array caching for Keyconfig
|
He 8000 llo @brusch I have implemented caching for the group as well expanded the same onto the collections. I got one more thing. I have again modified the KeyConfig, as there was an additonal bug I discovered during my tests. The KeyConfig.php implements another runtime caching (next to the Pimcore Runtime Caching) in the KeyConfig class using a local array which was toggled using the variable $cacheenabled. Whenever you request a KeyConfig using an id and have $cacheenbaled on true the if clause tries to retrieve a value from the array by the given index $id. This fails with an exception when the requested $id was not stored yet. Example:
If I request a KeyConfig with id 29 which is not yet stored in the array with cacheenabled = true it yíelds an exception
I'm not sure why the KeyConfig implements another local array caching in the first place while at the same time using the Runtime Cache. It is practically the same, right? I have remove this local array caching for now, however, if i miss something here I can put that back in, let me know. As well I have consolidated the cache getting, setting and removing into static functions for Key-,Group-, and CollectionConfig. This made the implementation more consisten across the three scopes. Thank You :) |
|
@brusch I have refactored the classes. They are now using the Trait in lib/Cache/RuntimeCacheTrait.php. I was not sure how to handle the Trait signature. Currently i left the standard signature which applies to myself but shouldn't be your Pimcore signature in here? Let me know if I have to change this. many thx |
|
@awroblewski thanks a lot, looks good to me now. The license header should be the one from Pimcore, see code suggestion. Thanks! |
Co-authored-by: Bernhard Rusch <brusch@users.noreply.github.com>
|
@brusch , just have seen this while writing the comment, all good now |
Pull request as per issue #11144
@brusch , as suggested I've created a static method to separate the cache key creation logic as per your given example. The cache keys for the classification store keys are now calculated using this method.
@weisswurstkanone , I've implemented the same for the GroupConfig, however, the GroupConfig did not use caching on behalf of the group name but just the id. So the current cache invalidation stays the same. Does it probably make sense to enable the same caching on behalf of the group name as it is done with the KeyConfig or was it left out on purpose?
Thank you