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.

CommentFileSizeAuthor
#1 searchpage_cache_tags-2241249-1.patch3.07 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
3.07 KB

The new SearchPageCacheTagsTest is modified from SearchPageTextTest.

jhodgdon’s picture

Status: Needs review » Postponed (maintainer needs more info)

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

Wim Leers’s picture

Status: Postponed (maintainer needs more info) » Needs review

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

jhodgdon’s picture

Status: Needs review » Needs work

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

Wim Leers’s picture

Title: Make search results pages cacheable: add the associated SearchPage's cache tag » First step in making search results pages cacheable: add the associated SearchPage's cache tag
Status: Needs work » Needs review

#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:

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

I've updated the issue title to make that even more explicit.

b) You say

I think any time NodeSearch plugin does anything in the index, all NodeSearch-based entities need to have their cached search pages invalidated

Which is absolutely correct! :) But again, that's why I said:

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

c) Once more:

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


So, let me explain the purpose of the issue again:

  1. This is a baby step: it surfaces a cache tag to the render layer that was previously not exposed (that for SearchPage entities). That doesn't get us anywhere, but it's a baby step in the right direction.
  2. To make search pages actually cacheable, we would of course indeed need to make sure that node/user/… cache tags are set, and there should be test coverage for it. But we're not yet enabling caching of search results pages.
  3. Any entity rendered by Entity API gets cache tags applied automatically. Since there's a "Search index" view mode for Node entities, I don't understand why those don't bubble up right now. But, again, fixing that is out of scope for this issue.
  4. Finally, indeed, it's questionable whether enabling caching on search results pages is worthwhile at all, because, indeed, every change to the search index might invalidate all search results pages, which would be highly inefficient. But, again, I'm not proposing to enable caching of search results pages; I'm merely saying that we should not prevent that, and one requirement for enabling it, is these 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.

jhodgdon’s picture

Issue summary: View changes

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

jhodgdon’s picture

Also, 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?

Wim Leers’s picture

Issue summary: View changes

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

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

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

  1. make sure that the SearchPage config entity's cache tag is indeed set on render arrays where the SearchPage entity is being used to affect rendering
  2. add test coverage to verify that this cache tag correctly bubbles up

Test 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 and ConfigEntityStorageTest 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.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Nice baby step. Thanks.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I 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

  • Commit 6541e3a on 8.x by catch:
    Issue #2241249 by Wim Leers: First step in making search results pages...
Wim Leers’s picture

Issue tags: -sprint

Yay, one more config entity that correctly sets its cache tag! Baby steps!

Status: Fixed » Closed (fixed)

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