As part of #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly, we added standardized cache tags to all entities in #2217749: Entity base class should provide standardized cache tags and built-in invalidation (with test coverage).
search.module's SearchPage
config entity type affects rendered content: that of search results pages.
In this issue, a cache tag is added to search results pages for the SearchPage entity. See comment #4 for other cache tags that would be needed in order to cache search results pages properly. See comment #5 for reasons for wanting this patch to be committed as a first step, just like it was done in other child issues of #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly.
Comment | File | Size | Author |
---|---|---|---|
#1 | searchpage_cache_tags-2241249-1.patch | 3.07 KB | Wim Leers |
Comments
Comment #1
Wim LeersThe new
SearchPageCacheTagsTest
is modified fromSearchPageTextTest
.Comment #2
jhodgdonUmmm... What does this patch actually accomplish, and what does the test actually test? It doesn't seem to be testing that the search results pages are actually being cached? And how would that work, given that they are paginated etc.?
Comment #3
Wim LeersPlease read up on cache tags in general (https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group... should be sufficient), and see https://drupal.org/node/2222835 in particular.
This will indeed allow us to cache search results pages. Search pages already should have cache tags for every single entity they list in their search results. This makes it so that whenever search page settings change (i.e. content ranking, label and path, like you can see at e.g.
/admin/config/search/pages/manage/node_search
), that all cached search pages will be invalidated also.Note that this does NOT enable caching of search pages, it merely ensures that the cache tag corresponding with the search page settings is set.
Comment #4
jhodgdonYeah, since I posted the comment above, I have learned more about cache tags...
So I don't think this patch is sufficient for really tagging search pages for caching:
a) I am not convinced that the search page would have cache tags for every entity listed in the results. How/where is that taking place, for the Core searches (User and Node search), or any other Search plugin? And would the next page(s) of results also have their entities tagged? That would be necessary, in the case that an edit of one of the later-page nodes would make that entity go away (which could at a minimum make the pager on the page different), or move that entity up/down in the results list due to rankings. I don't see any of this in the patch, and the test is not verifying that it is the case, or that updating a node/user would invalidate a search results page that returned that node/user as a result.
b) Even if the entity tags are present for entities that are actually in all pages of the results, search pages that use the index (such as the NodeSearch plugin) would need to have all their search pages invalidated whenever anything is added, removed, or updated in that plugin's search index. It's not just the nodes that are in the cached search results pages -- if new nodes are added to the system, they could have the same keywords in them, and therefore the search results previously returned for that keyword search would not be valid. So basically, I think any time NodeSearch plugin does anything in the index, all NodeSearch-based entities need to have their cached search pages invalidated. I do not see this patch doing that, or a test for it.
c) For searches using other plugins that don't use the index, like UserSearch plugin, an addition of a new user account would also potentially invalidate cached search results, if the new user matched cached keywords. I do not see anything in this patch that is doing this, or a test for it.
Comment #5
Wim Leers#4: Please actually read what I wrote. Maybe I wasn't clear enough? :) >90% of #4 is out-of-scope.
a) I never said that this added cache tags for all entities being listed. That's why I said:
I've updated the issue title to make that even more explicit.
b) You say
Which is absolutely correct! :) But again, that's why I said:
c) Once more:
So, let me explain the purpose of the issue again:
SearchPage
entities). That doesn't get us anywhere, but it's a baby step in the right direction.SearchPage
entity cache tags. Everything must be cache tagged correctly and completely in Drupal 8.All this patch does, is adding the
SearchPage
entity's cache tag to the search results pages; along with test coverage.Comment #6
jhodgdonThere's really no need to get so upset. Previous to my comment #4, you had not explained the limited scope of this issue, at least not in a way that was obvious to me. Thanks for clarifying. I've updated the issue summary accordingly.
I do not see why this is a critical issue, since it doesn't come close to what is needed to have valid cached search results pages... but I'm not going to get into a flame war by downgrading it, marking it "needs work" again, etc.
However, since I still don't see why it's OK to add a cache tag to a page, while not taking care to invalidate said cache tag when it needs to be invalidated, I'm going to leave it to someone more knowledgeable about the cache system to mark this RTBC.
Comment #7
jhodgdonAlso, do you think it would make sense in the current patch to add a comment saying that the cache tag that is added is currently only invalidated for SearchPage entity CRUD, or maybe a @todo comment saying that additional invalidation needs to be done by plugins?
Comment #8
Wim Leers#6:
This issue is critical because it's a blocker for #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly, which is critical.
This shows you still don't understand how it works, but in this case it is something you could not have known: #2217749: Entity base class should provide standardized cache tags and built-in invalidation introduced standardized entity cache tags for all entity types. All this patch does, is
SearchPage
config entity's cache tag is indeed set on render arrays where theSearchPage
entity is being used to affect renderingTest coverage to verify that the
SearchPage
config entity's cache tag is invalidated when necessary is already added in #2217749: Entity base class should provide standardized cache tags and built-in invalidation, in a generic way:ConfigEntityBaseUnitTest
andConfigEntityStorageTest
verify this.#7:
No, because — again — that is out of scope. All this issue aims to do is to make sure that we have complete test coverage for the
SearchPage
entity. That's it. We've done it this way for every other entity type as well, so let's please continue along the same pattern.I've opened an issue for doing precisely that though: #2265065: Second step in making search results pages cacheable: make sure each entity's search plugin bubbles up cache tags.
I'm not upset. How could you interpret #5 as being upset? Of course, if you ask me to repeat my explanation three times, I have to be increasingly explicit and clear ever next try, because you didn't get it the previous time.
(In #2, you marked this as NW without valid reason; you did not take the time to understand the patch, especially because all answers to the question in #2 were already in the patch. Then, in #4, you again asked questions that prove you didn't read what I wrote. So, by #5, it's actually the third time I have to explain things, so of course I am making things more explicit.)
Your update to the issue summary is incorrect, so I've fixed that too.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedNice baby step. Thanks.
Comment #10
catchI also wasn't sure why this was critical, however I figured it out in #2265065-7: Second step in making search results pages cacheable: make sure each entity's search plugin bubbles up cache tags.
Boils down to:
1. We don't have test coverage for search results + page cache + invalidation
2. #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly would break that test coverage, were it to exist (because we'll no longer invalidate the entire page cache every content update)
In terms of regressions, I don't think it's really release blocking, just major, but we need to clarify whether all regressions should really be release blocking or not elsewhere.
Agreed that the patch doesn't really make sense on its own, #2124957] isn't going to be sufficient either unless we widen the scope, but since that issue is also open and critical let's continue there.
Committed/pushed this one to 8.x
Comment #12
Wim LeersYay, one more config entity that correctly sets its cache tag! Baby steps!