Problem/Motivation

This is a follow-up issue to #3062346: Restore in search feature bundles and menus removed for performance reasons. The caching is set to permanent and one thing that still needs to be done is to add cache tags so that if a new bundle or menu is added, the cache clears. I believe every entity type has a '_list' suffix tag. For example, 'node_list'. Since we're dealing with config entities, I'm not sure if there's a 'node_type_list', etc.

If you have enough bundles that they are being limited, and therefore loaded into the search from JSON, and you add a new one that should appear in the JSON, I believe currently, it would not be added automatically because the cache wouldn't clear.

Proposed resolution

- either add cache tags for the various config entity lists, or use some hook to invalidate the cache when new bundles or menus are added.

Remaining tasks

- find solution and add it.

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

CommentFileSizeAuthor
#9 admin-toolbar-search-cache-tags-3062729-9.patch7.37 KBoknate
#9 3062729-interdiff--3-9.txt595 bytesoknate
#3 admin-toolbar-search-cache-tags-3062729-2.patch7.36 KBoknate
#3 admin-toolbar-search-cache-tags-3062729--TEST-ONLY.patch6.03 KBoknate
#2 admin-toolbar-search-cache-tags-3062729-2.patch102.25 KBoknate
#2 admin-toolbar-search-cache-tags-3062729--TEST-ONLY.patch102.06 KBoknate

Comments

oknate created an issue. See original summary.

oknate’s picture

oknate
Location Greater New York City Area
commented
Status: Active » Needs review
StatusFileSize
new admin-toolbar-search-cache-tags-3062729--TEST-ONLY.patch102.06 KB
new admin-toolbar-search-cache-tags-3062729-2.patch102.25 KB

Here's a fix and test coverage, as well as a test only patch that demonstrates the issue.

Unfortunately, there's no option to run automated tests against 8.x-2.x branch. I think that needs to be set up within the project configuration.

#3062914: Update project settings to allow testing patches against 8.x-2.x branch

oknate’s picture

oknate
Location Greater New York City Area
commented
StatusFileSize
new admin-toolbar-search-cache-tags-3062729--TEST-ONLY.patch6.03 KB
new admin-toolbar-search-cache-tags-3062729-2.patch7.36 KB

Reroll. The patches in #2 were incorrectly diffed against 8.x-1.x instead of 8.x-2.x so they are completely wrong.

These are better.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented

I just added the tests

The last submitted patch, 3: admin-toolbar-search-cache-tags-3062729--TEST-ONLY.patch, failed testing. View results

oknate’s picture

oknate
Location Greater New York City Area
commented

Great, thanks.

I should maybe add test coverage for the menus in search. But I don't want to hold back the caching fix for test coverage.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented
Status: Needs review » Needs work 
+++ b/admin_toolbar_tools/src/SearchLinks.php
@@ -190,6 +192,7 @@ class SearchLinks {
+      Cache::mergeTags($cache_tags, ['config:menu_list']);

Here you don't use the returned cache tags array, this is an error or we don't need this line?

oknate’s picture

oknate
Location Greater New York City Area
commented

Good catch. That's why I should probably add test coverage for the menus too. I tested with the bundles.

It should be
$cache_tags = Cache::mergeTags($cache_tags, ['config:menu_list']);

oknate’s picture

oknate
Location Greater New York City Area
commented
Status: Needs work » Needs review
StatusFileSize
new 3062729-interdiff--3-9.txt595 bytes
new admin-toolbar-search-cache-tags-3062729-9.patch7.37 KB

Here's an update. I haven't tested the change. I can work on test coverage for the menus tonight.

  • adriancid committed 013179c on 8.x-2.x authored by oknate 
    Issue #3062729 by oknate, adriancid: Search Extra Links json caching:...
adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented
Status: Needs review » Fixed

thanks @oknate, we need this in the 8.x-1.x branch?

oknate’s picture

oknate
Location Greater New York City Area
commented

At this point, no, this doesn't apply to 8.x-1.x.

If you backport #3061837: Limit the number of menus to display in the drop-down admin menu. and #3061853: Do not load more than 10 bundles per entity type, then we'd need this after backporting #3062346: Restore in search feature bundles and menus removed for performance reasons.

I like the performance boost in the 8.x-2.x branch, but until it's configurable and there's an upgrade path, I don't don't think 3061837 and 3061853 should go into 8.x-1.x.

I think a lot of sites depend on those bundle links.

Status: Fixed » Closed (fixed)

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