Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
SearchPluginManager needs a $this->setCacheBackend() in the constructor as other have. Otherwise, the annotations are re-parsed on every request.
Comment | File | Size | Author |
---|---|---|---|
#12 | search-plugin-definitions-cached.patch | 1.7 KB | Bladedu |
#5 | search-plugin-definitions-cached.patch | 1.69 KB | Bladedu |
#5 | interdiff.txt | 581 bytes | Bladedu |
#4 | search-plugin-definitions-cached.patch | 1.42 KB | Bladedu |
Comments
Comment #1
BerdirTagging.
Comment #2
pwolanin CreditAttribution: pwolanin commentedYes, I noticed this too while rolling #2087965: The SearchPluginManager needs to provide an alter hook for its discovery. Since the 2 patches would conflict, maybe let's finish that 1st.
Comment #3
BladeduComment #4
BladeduThis patch should resolve the issue.
Comment #5
BladeduForgot to add the namespaces of CacheBackendInterface and LanguageManager. This should be fine.
Comment #6
jhodgdonI took a look at what some other plugin managers are doing... Example from BlockManager:
So maybe we should be saying 'search_plugins' in the call here, rather than just 'search'?
I'm also a bit confused about what's correct for the services.yml file. Block says '@cache.block' and then it sets up its own cache. Do we need to do that here as well? I'm a bit ignorant on all of this...
Comment #7
BerdirYou happened to look at almost the only one that does use a specific bin. Using the default one is fine, cache key also IMHO also fine, as long as it's unique...
Note that this will conflict with the issue mentioned in #2, but I think this one is a pretty severe Performance regression, so IMHO more important. I'd also be just fine with merging them into a single issue.
Comment #8
pwolanin CreditAttribution: pwolanin commentedI think I agree with jhodgdon that the cache key prefix should be a little more specific - otherwise it looks ready.
Comment #9
BerdirOk, let's go for search_plugins then?
Comment #10
BladeduIt's fine by me to go for search_plugin, but I propose that we also fix the other plugins to be consistent with the naming convention. I can open a new issue for that and provide a patch for it.
Comment #11
BerdirMaybe, feel free to open an issue, but such issues often result in discussions, because it's not the same. Renaming for example the entity manager cache id would be weird ;)
Let's just fix this and get this in, I just want to have the caching fixed asap :)
Comment #12
BladeduRenamed the cache key prefix to 'search_plugins'. I'll create another issue and I'll propose another patch for the naming conventions of other cache key prefixes.
Comment #13
BerdirOk, looks good to me, let's do this :)
Comment #14
jhodgdonThanks for the explanations Berdir, and the patching Bladedu. Looks good to me too!
Comment #15
catchThis never gets cleared anywhere. Should it be, or is that only going to happen on module install/uninstall anyway?
Comment #16
BerdirI think that's fine, many plugin managers work like that. A call to clearCachedDefinitions() would take care of it, and it's not like you're changing them "frequently" like views plugins.
Comment #17
catchOK works for me.
Committed/pushed to 8.x, thanks!