SearchPluginManager needs a $this->setCacheBackend() in the constructor as other have. Otherwise, the annotations are re-parsed on every request.

CommentFileSizeAuthor
#12 search-plugin-definitions-cached.patch1.7 KBBladedu
#5 search-plugin-definitions-cached.patch1.69 KBBladedu
#5 interdiff.txt581 bytesBladedu
#4 search-plugin-definitions-cached.patch1.42 KBBladedu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +Performance

Tagging.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Yes, 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.

Bladedu’s picture

Bladedu
he/him
Primary language Italian
Location Italy
CreditAttribution: Bladedu commented
Assigned: Unassigned » Bladedu
Bladedu’s picture

Bladedu
he/him
Primary language Italian
Location Italy
CreditAttribution: Bladedu commented
Status: Active » Needs review
FileSize
search-plugin-definitions-cached.patch1.42 KB

This patch should resolve the issue.

Bladedu’s picture

Bladedu
he/him
Primary language Italian
Location Italy
CreditAttribution: Bladedu commented
FileSize
interdiff.txt581 bytes
search-plugin-definitions-cached.patch1.69 KB

Forgot to add the namespaces of CacheBackendInterface and LanguageManager. This should be fine.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I took a look at what some other plugin managers are doing... Example from BlockManager:

  $this->setCacheBackend($cache_backend, $language_manager, 'block_plugins');

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...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

You 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.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

I think I agree with jhodgdon that the cache key prefix should be a little more specific - otherwise it looks ready.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Ok, let's go for search_plugins then?

Bladedu’s picture

Bladedu
he/him
Primary language Italian
Location Italy
CreditAttribution: Bladedu commented

It'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.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Maybe, 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 :)

Bladedu’s picture

Bladedu
he/him
Primary language Italian
Location Italy
CreditAttribution: Bladedu commented
Status: Needs work » Needs review
FileSize
search-plugin-definitions-cached.patch1.7 KB

Renamed 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.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Ok, looks good to me, let's do this :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks for the explanations Berdir, and the patching Bladedu. Looks good to me too!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

This never gets cleared anywhere. Should it be, or is that only going to happen on module install/uninstall anyway?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I 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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

OK works for me.

Committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.