Problem/Motivation

At some point, the search block could not be made cacheable. It had to do with CSRF tokens. But… the search form is a GET form, which doesn't use CSRF tokens. So there's no reason not to make it cacheable.

Proposed resolution

Make it cacheable.

Remaining tasks

None.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#3 search_block_cacheable-2497307-3.patch3.12 KBwim leers
#1 search_block_cacheable-2497307-1.patch683 byteswim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Issue tags: +Quickfix
StatusFileSize
new search_block_cacheable-2497307-1.patch683 bytes

Status: Needs review » Needs work

The last submitted patch, 1: search_block_cacheable-2497307-1.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new search_block_cacheable-2497307-3.patch3.12 KB

Root cause: missing cache tags for the config that the search block form depends upon.

Requires an unfortunate solution because SearchPageRepository is designed in an unfortunate way.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Awesome!!

+++ b/core/modules/search/src/Form/SearchBlockForm.php
@@ -85,6 +109,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    // SearchPageRepository::getDefaultSearchPage() depends on search.settings.
+    $this->renderer->addCacheableDependency($form, $this->configFactory->get('search.settings'));

I'm curious whether you've thought about just doing $form['#cache']['dependencies'][] and handle the rest internally?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I'm curious whether you've thought about just doing $form['#cache']['dependencies'][] and handle the rest internally?

No. And first I thought this was a stroke of utter genius. But then I realized that it may mean we end up serializing objects. Then again, the rendering could take care of that.

So I think it it actually may be a stroke of genius :) :) :) @dawehner++

catch’s picture

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

Committed/pushed to 8.0.x, thanks!

  • catch committed ba05c74 on 8.0.x 
    Issue #2497307 by Wim Leers: Search block marks itself uncacheable for...

Status: Fixed » Closed (fixed)

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