Make POST requests render cacheable

Assuming this works it will be a significant performance improvement for every form (including AJAX forms), so bumping to major.

Added an MR that does this:

Add a 'form' cache tag to all forms.
During a POST request, look for deny-listed cache tags like 'form'.
If on a cache get (during a POST) you encounter such a cache tag, the item is treated as a cache miss (like we do now for render cache gets on POST requests).
All other cache items can be returned from the render cache.

We would continue to not SET render cache data during a POST request.

And a surprisingly low number of things exploded. Haven't manually tested yet, would be a good candidate for verfiying #3396196: Separate cache operations from database queries in OpenTelemetry and assertions.

The remaining test failure is a strange one, the error is this (full backtrace is important, see below):

    There was 1 error:
    
    1)
    Drupal\Tests\page_cache\Functional\PageCacheTagsIntegrationTest::testPageCacheTags
    PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught PDOException:
    SQLSTATE[42S02]: Base table or view not found: 1146 Table
    'mysql.test18048985path_alias' doesn't exist in
    /builds/project/drupal/core/lib/Drupal/Core/Database/StatementWrapperIterator.php:111
    Stack trace:
    #0
    /builds/project/drupal/core/lib/Drupal/Core/Database/StatementWrapperIterator.php(111):
    PDOStatement->execute(Array)
    #1
    /builds/project/drupal/core/lib/Drupal/Core/Database/Connection.php(826):
    Drupal\Core\Database\StatementWrapperIterator->execute(Array, Array)
    #2
    /builds/project/drupal/core/lib/Drupal/Core/Database/Query/Select.php(525):
    Drupal\Core\Database\Connection->query('SELECT "base_ta...', Array, Array)
    #3
    /builds/project/drupal/core/modules/path_alias/src/AliasRepository.php(69):
    Drupal\Core\Database\Query\Select->execute()
    #4
    /builds/project/drupal/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php(32):
    Drupal\path_alias\AliasRepository->lookupBySystemPath('/node/1', 'en')
    #5
    /builds/project/drupal/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php(34):
    Drupal\path\Plugin\Field\FieldType\PathFieldItemList->computeValue()
    #6
    /builds/project/drupal/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php(43):
    Drupal\path\Plugin\Field\FieldType\PathFieldItemList->ensureComputedValue()
    #7
    /builds/project/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php(550):
    Drupal\path\Plugin\Field\FieldType\PathFieldItemList->getValue()
    #8 [internal function]: Drupal\Core\Entity\ContentEntityBase->__sleep()
    #9 Standard input code(84): serialize(Array)
    #10 Standard input code(123): __phpunit_run_isolated_test()
    #11 {main}
    
    Next Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base
    table or view not found: 1146 Table 'mysql.test18048985path_alias' doesn't
    exist: SELECT "base_table"."id" AS "id", "base_table"."path" AS "path",
    "base_table"."alias" AS "alias", "base_table"."langcode" AS "langcode"
    FROM
    "test18048985path_alias" "base_table"
    WHERE ("base_table"."status" = :db_condition_placeholder_0) AND
    ("base_table"."path" LIKE :db_condition_placeholder_1 ESCAPE '\\') AND
    ("base_table"."langcode" IN (:db_condition_placeholder_2,
    :db_condition_placeholder_3))
    ORDER BY "base_table"."langcode" ASC, "base_table"."id" DESC; Array
    (
        [:db_condition_placeholder_0] => 1
        [:db_condition_placeholder_1] => /node/1
        [:db_condition_placeholder_2] => en
        [:db_condition_placeholder_3] => und
    )
     in
    /builds/project/drupal/core/modules/mysql/src/Driver/Database/mysql/ExceptionHandler.php:56
    Stack trace:
    #0
    /builds/project/drupal/core/lib/Drupal/Core/Database/Connection.php(858):
    Drupal\mysql\Driver\Database\mysql\ExceptionHandler->handleExecutionException(Object(PDOException),
    Object(Drupal\Core\Database\StatementWrapperIterator), Array, Array)
    #1
    /builds/project/drupal/core/lib/Drupal/Core/Database/Query/Select.php(525):
    Drupal\Core\Database\Connection->query('SELECT "base_ta...', Array, Array)
    #2
    /builds/project/drupal/core/modules/path_alias/src/AliasRepository.php(69):
Drupal\Core\Database\Query\Select->execute()
#3
/builds/project/drupal/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php(32):
Drupal\path_alias\AliasRepository->lookupBySystemPath('/node/1', 'en')
#4
/builds/project/drupal/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php(34):
Drupal\path\Plugin\Field\FieldType\PathFieldItemList->computeValue()
#5
/builds/project/drupal/core/lib/Drupal/Core/TypedData/ComputedItemListTrait.php(43):
Drupal\path\Plugin\Field\FieldType\PathFieldItemList->ensureComputedValue()
#6
/builds/project/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php(550):
Drupal\path\Plugin\Field\FieldType\PathFieldItemList->getValue()
#7 [internal function]: Drupal\Core\Entity\ContentEntityBase->__sleep()
#8 Standard input code(84): serialize(Array)
#9 Standard input code(123): __phpunit_run_isolated_test()
#10 {main}
  thrown in
/builds/project/drupal/core/modules/mysql/src/Driver/Database/mysql/ExceptionHandler.php
on line 56
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97

What's happening is that the path alias field item is trying to check for an alias, when the path_alias table doesn't exist.

First of all I thought this might be that the MR had broken the installer somehow, so that the path_alias table never got created. But I added some debug to the content entity SQL schema and confirmed the table was created.

This means that instead, it's requesting the alias after the database table has been removed, which can happen in phpunit teardown, I guess...?

As you can see from the backtrace, what causes this is a call to serialize(), although the backtrace prior to serialize() doesn't tell us what actually caused the serialization of the entity. So... I still haven't worked out what exactly is different between HEAD and the MR that would cause this.

However, since path aliases are a computed field, it is completely pointless to serialize the value, so I added ComputedFieldItemListInterface and had PathAliasItemList implement it.

After that, the test still failed, but it was failing on the cache tag assertion instead.

So then I realised, that this isn't actually a real fatal error caused by the MR, but a fatal error caused by a test failure caused by the MR. When the test failed, it was cleaning up the database, and then the node was sleeping. I don't know if there's something we can do about that in phpunit/our test base classes or not, it's definitely very confusing.

Opened a new issue for the computed field stuff because that seems worthwhile anyway #3397424: Don't serialize computed fields.

Pushed a commit for the actual test fix which is just to add the extra cache tag. This might be green now.

It might be a fluke, but the core test run that usually takes 10+ minutes finished in 8 minutes 59 seconds: https://git.drupalcode.org/project/drupal/-/pipelines/40707

Tagging this with 'needs profiling' although my plan is to add logging to cache get/set to prove specifically that caching is working so we can see actual cache ID sets and gets, and ideally to codify that with #3396196: Separate cache operations from database queries in OpenTelemetry and assertions (although I would add the performance test coverage on that issue, not this one, even if it lands first we could modify the assertions in the MR here to show the improvement).

One thing I didn't realise when writing this issue up but did since, is that we are essentially guaranteed an almost perfect cache hit rate here:

When you view a form, you prime the render cache for your user, permissions, language, path etc on that page, when you submit the form, you submit it to the same URL, that means the POST request will be fetching render cache items with the same context as the previous GET request, so they will all be hits. There will be situations where the whole page was cached a while ago in page cache/dynamic page cache and some of the individual render cache items have expired in the meantime, but short of other requests invalidating caches it should be pretty good.

I should mention the original idea here was all Fabianx's, I just agreed with it, wrote it up, and am implementing it :) Amazing how easy this is (so far) compared to the minefield which would have been trying to get #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs done.

One self-review point, we might want to add a constant for render_cache_form somewhere so that we can document it, but... where would that go? RenderCacheInterface maybe?

For test coverage, I'm hoping to implement #3396196: Separate cache operations from database queries in OpenTelemetry and assertions and add test coverage of HEAD there, and then add a combined branch of that issue + this one with the test coverage adjusted. It's an ideal scenario to validate the approach in that issue since it's not often we get something which should so dramatically change the numbers on one request. The idea would be to submit the user login form (or any form, but that's an easy one), then assert on the count of the cumulative cache hits and misses from the form submission and subsequent page render. We could also go further by doing #3352851: Allow assertions on the number of database queries run during tests to compare database queries too.

We can probably add explicit test coverage for the behaviour in RenderCacheTest too.

I added initial database query coverage in #3352851: Allow assertions on the number of database queries run during tests and didn't see any difference in queries executed. This was because PlaceholderingRenderCache also prevented caching on POST requests.

https://git.drupalcode.org/project/drupal/-/merge_requests/5100/diffs?co... fixes that and only introduces unit test failures.

This actually increases the number of database queries executed, but I confirmed at least 20 of them were cache_render tablequeries and HITs so that shows render caching working.

I am seeing the views pager query run despite it being render cacheable, with node grants query cache contexts applied etc. causing a miss, that looks like it might be an actual bug but haven't tracked down exactly what it is yet. Potentially fixing that would change quite a lot.

I've also opened an additional MR that combines this issue with the test coverage.

This should be green again. I opened #3400596: Views adds cache contexts inconsistently to investigate render cache misses on views pagers on POST which I found working on #3352851: Allow assertions on the number of database queries run during tests.

You can now see the cache gets increase in the user login block test added in #3352851: Allow assertions on the number of database queries run during tests, and also that we don't set any new cache items (which is expected since we've retained not writing back to caches on POST requests).

The database queries increase too - some of this might be cache tags, some of it might be #3400596: Views adds cache contexts inconsistently - going to try to get something going on that issue to see what the impact is.

Rebased.

Rebased again after #3396196: Separate cache operations from database queries in OpenTelemetry and assertions.

Rebased again.

Added a change record: https://www.drupal.org/node/3420901

Closed the draft MR, rebased, added the follow-up to match the @todo, and updated the issue summary. No real code changes apart from different numbers in StandardPerformanceTest, so moving back to RTBC.

This needed another rebase after #3421164: Log every individual query in performance tests and related issues, but in the best possible way - it now shows the menu tree query disappearing and the increases in cache gets and cache tag gets are much easier to see.

I've updated the issue summary (not the CR yet, but will do so in a bit) with the explanation of why lazy builders are important, but extra reply here:

All forms are currently max-age:0 until #2578855: Form tokens are now rendered lazily, allow forms to opt in to be cacheable lands, at least unless you really go out of your way to override core behaviour (I haven't tried this since Drupal 7 so not sure how doable it currently is).

Due to bubbling, any render array including a form also becomes max-age:0. This means that something like the comment form, if it's not in a lazy builder, would cause the node and all the comments to be uncacheable too. The comment form is in a lazy builder for this reason: https://api.drupal.org/api/drupal/core%21modules%21comment%21src%21Comme...

When a form is rendered inside a lazy builder (all blocks are rendered inside lazy builders), this short-circuits the bubbling of max-age:0, and any 'parent' render arrays become cacheable again - because for the parent render array/cache item, the form is just a placeholder now due to auto-placeholdering. Same with any other render array, it's just that forms default to uncacheable whereas everything else doesn't.

So to get the full benefit of this issue on POST requests, deeply nested forms in render arrays need to be in lazy builders - however this is also the case for render caching on GET requests too.

When a form is in a block (or lazy builder), even though the form itself might be uncacheable, because the other stuff on the page already is, then this issue 'just works' as can be seen in the performance test changes.

edited to add: when a form in the main content area is a form controller - like the node add page, it's the first render array that gets rendered anyway on the POST request, so this issue makes no difference either way - this is why only the testloginBlock and not the testLogin (which submits the full login page form) see a difference. I was almost disappointed when I realised this, but of course it's good that we don't render blocks on those pages on a form submission and just means this issue will only be a performance improvement for some forms and neutral for others.

Updated the CR with more information based on the above (and with the new tag name).

Pushed an update to the comment that explains we do this for form discovery and removes the @todo. We could still try to do the 'submit to dedicated URL' issue, it's just about 20 times harder (maybe even more than that) than this one, but for me @todo usually implies something small, not rewriting significant parts of core.

Especially #39 seems to now warrant a comment along the lines of: "We do not set cache items during POST requests so there's no point in creating placeholders."

There's an additional reason to not create placeholders, from a performance perspective possibly the more important one as it relates to this issue.

The idea of placeholders is that we render as much as possible as quickly possible, skipping the placeholders, then at the end (of the normal page response, or via bigpipe replacements), render the placeholders later.

If we've submitted a form, and that form was in a placeholder, we don't want to defer rendering of the form, in fact we want to render the form as soon as possible, so that we can submit the form and redirect. I've updated the comment with that info.

I haven't gone through the other places in core that mention https://www.drupal.org/node/2367555 though, but agreed it's probably a good idea to do that - leaving needs review and will do some grepping later on.

Went through the @todos linking to https://www.drupal.org/node/2367555

Some are still valid, the weird architecture of form submissions is still an issue (if not the performance of it as much) once this lands, but I tried to add longer comments to clarify what's going on.

The one in BlockContentCacheTagsTest is outdated - didn't explicitly check whether it's because of this issue but we can definitely remove it here anyway, just deletions for that.

Removing the needs profiling tag because performance tests have come on enough to show how this improves things without explicit manual profiling, which I have to say I'm extremely happy about - it's always annoyed me that we remember to require profiling on performance improvements and forget it on all the issues that introduce regressions.

Had one extra thought here:

BigPipe because it's dealing with sending in chunks definitely needs to be disabled on POST requests still.

However, I think with this approach it's actually fine to enable placeholdering itself, and it might result in a higher render cache hit rate on some pages because they'll be more similar to GET requests. It might mean that forms get rendered later in the request but if we get a higher cache hit rate that would be worth it.

This is a very small change, so went ahead and did it - only had to update the unit test which specifically enforces the old behaviour.

I think you're right, so it doesn't break anything but also doesn't improve anything either, reverted those two commits, but tried to update the comment again to explain why there's no benefit.

Rebased again - had to resolve some merge conflicts but I think the changes are small enough to re-RTBC.

Updated the issue summary.

@alexpott asked if the extra cache gets are really worth it - I think they are because they represent render cache gets vs. actual rendering (as long as they're cache hits) and we know that render caching saves a tonne of CPU time. He suggested using grafana to compare timings, which is a good idea and easy enough to do locally (the standard tests aren't on the public graphs yet, just the umami ones). Looking at that, I realised the user login tests don't have any node at all so we're testing the performance of the empty front page view which is not very realistic, so adding one node in setUp() to make it moderately more realistic. Makes me wonder if we should move this coverage to umami and enable the login block there for this test instead though.

All of that means I haven't actually got proper comparison numbers yet, maybe tomorrow.

Added a draft MR that just creates a node on the front page of standard, so that we can then test the login block with that node instead of empty views text.

As you can see, that massively increases the cache get counts on the front page, so that the diff between the cache gets is pretty much zero compared to the change here (just some more cache tag checks which you'd expect with more render caching), while we drop a query https://git.drupalcode.org/project/drupal/-/merge_requests/6946/diffs

This explains why the MR is adding more cache gets than we expect - the test coverage is doing less cache gets than we'd expect because no nodes are being loaded and rendered on the front page.

This explains why the cache gets were so much higher - not testing quite the right thing. It's OK for the standard login test before this change, since that's only testing the login really, but for this issue we need a slightly more realistic scenario.

I ran the tests three times each between the two branches, and the best of three is 217ms before the change and 198ms after the change. I did this process twice, and got 214ms vs 199ms the first time - but quite a bit of variation above that as you can see, which I put down to doing things on my laptop/ddev.

Before trace:

After trace:

Three runs vs. three runs (with the change is the top three in the list):

This is just rendering a teaser with a single field. If we added a load of different fields, more entities etc. that'd be a lot more rendering and should be a lot more time saved by the MR, but I think that needs Umami rather than standard - it might be worth moving the login test coverage over there in a separate issue.

Added a release note to the 10.3.0 release notes draft and tagging for release highlights as a performance improvement.

Also opened #3426303: Improve the standard performance login test.