Make POST requests render cacheable

Created on 20 October 2023, 4 months ago
Updated 1 March 2024, 1 day ago

Problem/Motivation

Opening this as a replacement issue for 📌 Allow both AJAX and non-AJAX forms to POST to dedicated URLs Postponed since it's a completely new approach to the same problem, will mark that issue postponed on this one. Mostly summarising in-person (and slack) discussions with Fabianx at DrupalCon Lille here.

Render caching is completely disabled on POST requests, because our form detection logic relies on re-rendering the entire page to discover where the submitted form is in the render array.

Steps to reproduce

Proposed resolution

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.

This will allow the current 're-render the forms to find the one that was submitted' logic to work, without changes, but without having to render the entire page - we'd only do rendering for any forms.

One important thing is that big_pipe and other placeholder render strategies needs to be disabled - so placeholders need to be executed as part of the main page request still.

Once the above is implemented, this will already start working for forms in blocks, so is independently useful.

For form controllers executed in the main page content (e.g. the node add form), the impact of this is neutral, because on those POST requests, the form gets rendered first before the rest of the page anyway, and redirects before anything else is rendered, and the render cache tag prevents the form from being cached.

For other forms in the main page content, if the form has max-age:0 (which is the case for all forms until 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review lands) it will prevent caching of render arrays via bubbling, so there will be no effect on the overall caching situation compared to now.

If the form is in a lazy builder, then other render arrays in the main page content can be cached, because the max-age: 0 won't bubble.

Couple of questions that came to mind but I didn't get a chance to discuss with Fabianx:

When we submit the form, by 'breaking' the render cache for it on POST, we'll be treating the existing cache item as invalid. Two questions for this - do we need to prevent the render cache item being set back, and if so, is it OK if we don't actually invalidate the form at all, and leave whatever's in the render cache valid for the next GET request? I think that probably is OK unless the form has a different cache tag that should also invalidate it, and that cache tag would be invalidated when we actually submit the form, which will be after it's been (re-) rendered, but would be good to validate this (no pun intended).

Fabianx:

Great question!

- For Ajax FORMs the form execution is completely interrupting the page render execution by throwing the Exception, so it will not in any case store cache items.

- For normal forms it's more tricky, we want the result definitely not be cached as it could contain e.g. validation messages.

- So on POST we don't do render cache sets, is probably a really good rule. (Now we do neither get nor set, after this patch we only allow GET under certain circumstances) [updated above]

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

RTBC

Version

11.0 🔥

Component
Form 

Last updated about 3 hours ago

Created by

🇬🇧United Kingdom catch

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • 🇬🇧United Kingdom catch
  • 🇬🇧United Kingdom catch

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

  • last update 4 months ago
    30,367 pass, 8 fail
  • last update 4 months ago
    30,367 pass, 8 fail
  • last update 4 months ago
    30,425 pass, 4 fail
  • Status changed to Needs work 4 months ago
  • 🇬🇧United Kingdom catch

    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 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed .

  • last update 4 months ago
    30,427 pass, 2 fail
  • last update 4 months ago
    30,427 pass, 2 fail
  • last update 4 months ago
    30,433 pass, 2 fail
  • Status changed to Needs review 4 months ago
  • 🇬🇧United Kingdom catch

    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 📌 Don't serialize computed fields Needs review .

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

  • 🇬🇧United Kingdom catch

    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 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed (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.

  • 🇬🇧United Kingdom catch

    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 📌 Allow both AJAX and non-AJAX forms to POST to dedicated URLs Postponed 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?

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    This is awesome.

    We might be able to add https://github.com/previousnext/drupal-test-utils/blob/main/src/Traits/E... to BrowserTestBase once this is done.

    It has picked up several regressions in caching in contrib modules.

    It would also prevent more instances of #3232018: Trigger an error when using \Drupal\Core\Cache\RefinableCacheableDependencyTrait::addCacheableDependency with a non CacheableDependencyInterface object

  • 🇬🇧United Kingdom catch

    For test coverage, I'm hoping to implement 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed 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 📌 Allow assertions on the number of database queries run during tests RTBC to compare database queries too.

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

  • Merge request !5298Draft: Resolve #3395776 "With tests" → (Closed) created by catch
  • 🇬🇧United Kingdom catch

    I added initial database query coverage in 📌 Allow assertions on the number of database queries run during tests RTBC 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 tags applied etc., 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.

  • Status changed to Needs work 4 months ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Status changed to Needs review 4 months ago
  • 🇬🇧United Kingdom catch

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

  • 🇺🇸United States smustgrave

    @catch should profiling happen before tests?

  • 🇬🇧United Kingdom catch

    You can now see the cache gets increase in the user login block test added in 📌 Allow assertions on the number of database queries run during tests RTBC , 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 📌 Views adds cache contexts inconsistently Active - going to try to get something going on that issue to see what the impact is.

  • Status changed to Needs work 2 months ago
  • 🇺🇸United States smustgrave

    Not 100% sure how to review this one but appears to be unmergable now.

  • Status changed to Needs review 2 months ago
  • 🇬🇧United Kingdom catch

    Rebased.

  • 🇺🇸United States smustgrave

    So don't want this one to stale but am unsure how to test or profile it.

  • Status changed to Needs work about 1 month ago
  • The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Pipeline finished with Success
    about 1 month ago
    Total: 558s
    #84372
  • Status changed to Needs review about 1 month ago
  • 🇬🇧United Kingdom catch

    Rebased again.

  • Status changed to Needs work 19 days ago
  • 🇺🇸United States smustgrave

    This is one I'm leaning on the expertise of @catch as a committer.

    Feel bad this has been endlessly rebased but couldn't find anyone to review so going to give a shot.

    So code wise nothing is standing out to me. But am tagging for a CR incase maybe contrib/custom modules want to start leveraging the new cache tags for their own purposes.

  • Status changed to Needs review 19 days ago
  • 🇬🇧United Kingdom catch
  • Status changed to RTBC 19 days ago
  • 🇺🇸United States smustgrave

    For me CR looks good.

  • Status changed to Needs work 10 days ago
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Needs a reroll, there are two MRs can we get one hidden/closed.
    Can we also get an issue summary update

    Thanks

  • Pipeline finished with Failed
    9 days ago
    Total: 542s
    #101240
  • Pipeline finished with Success
    9 days ago
    #101278
  • Status changed to RTBC 9 days ago
  • 🇬🇧United Kingdom catch

    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.

  • 🇬🇧United Kingdom catch

    This needed another rebase after 📌 Log every individual query in performance tests Needs work 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.

  • Pipeline finished with Canceled
    5 days ago
    #104353
  • Pipeline finished with Success
    5 days ago
    Total: 484s
    #104358
  • Status changed to Needs review 4 days ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    Very cool to see this happening at last! 😊

    Left a suggestion on the MR that would make the follow-up unnecessary.

    To actually make it work for forms in the main page content, each form needs to be built inside a lazy builder, probably need to opt-in one by one. Forms can still be max-age=0 if they're in a lazy builder (but could also be cached), other forms that are max-age 1 but not in a lazy builder will work too, because they're also not cached.

    This is a pretty important limitation that should be explicitly mentioned in the change record too?

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Trying to wrap my head around what's going on in here and it's really cool to see the menu query go away. If I understand correctly, it's because we can now get the menu from the render cache where we would before rerender everything that so happened to be on the same page as a form.

    Having said that, I'm not 100% clear on why it only works for forms with lazy builders. Is it because the "special cache tag" would otherwise bubble up and therefore cause a chain reaction of us not setting the other stuff on the page in the render cache or am I misreading this?

    Fully agree with Wim on finding a cleaner way to indicate which tags should be skipped on POST, even though the current suggestion is rather verbose :)

  • Status changed to Needs work 4 days ago
  • 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @kristiaanvandeneynde reviewed too, and confirmed my concerns, so moving back to .

  • Pipeline finished with Failed
    4 days ago
    Total: 174s
    #104849
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I'm re-reading the Renderer class for the umpteenth time in my career to figure out why lazy builders are involved in this, but would be nice if someone could deny or confirm what I assumed in #34 :)

  • 🇬🇧United Kingdom catch
  • Pipeline finished with Failed
    4 days ago
    Total: 184s
    #104853
  • 🇬🇧United Kingdom catch

    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 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review 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.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Thanks so much for #38. Really clear explanation.

    P.S.: Do we also need to update Renderer? It still checks for the method being cacheable and references the other issue.

        // If instructed to create a placeholder, and a #lazy_builder callback is
        // present (without such a callback, it would be impossible to replace the
        // placeholder), replace the current element with a placeholder.
        // @todo remove the isMethodCacheable() check when
        //   https://www.drupal.org/node/2367555 lands.
        if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE && $this->requestStack->getCurrentRequest()->isMethodCacheable()) {
          if (!isset($elements['#lazy_builder'])) {
            throw new \LogicException('When #create_placeholder is set, a #lazy_builder callback must be present as well.');
          }
          $elements = $this->placeholderGenerator->createPlaceholder($elements);
        }
    
  • Pipeline finished with Success
    4 days ago
    Total: 512s
    #104900
  • Status changed to Needs review 4 days ago
  • 🇬🇧United Kingdom catch

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

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Re #39: I know that it makes no sense to placeholder stuff on POST requests if we're not gonna cache it anyways, but maybe we can remove the @todo in the renderer? At this point it doesn't seem we're going to fix the issue it's pointing to. We can probably keep the isMethodCacheable() check now.

  • 🇬🇧United Kingdom catch

    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.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    For me all of this looks good and I'm willing to RTBC based on what I can see in the MR.

    I'm just worried that we might be missing something given that, after this change, there's still about 7-8 places in core that mention " https://www.drupal.org/node/2367555 " and I'm not sure if all of these have been checked to see if said comments and/or code still make sense after this change.

    Some of them will definitely be about that exception FormBuilder is throwing, but some really looks like we can remove a reference to said issue now. 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."

    In #42 you mentioned changing a comment about form discovery but I don't see that in the commits, so maybe you already addressed my concerns but forgot to push it? :)

  • 🇬🇧United Kingdom catch

    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.

  • Pipeline finished with Success
    4 days ago
    #105101
  • 🇬🇧United Kingdom catch

    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.

  • Pipeline finished with Failed
    2 days ago
    Total: 161s
    #107287
  • Pipeline finished with Success
    2 days ago
    Total: 592s
    #107290
  • Pipeline finished with Failed
    2 days ago
    Total: 487s
    #107300
  • Pipeline finished with Success
    2 days ago
    Total: 541s
    #107354
  • 🇬🇧United Kingdom catch

    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.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    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.

    I'm not sure how. When we're on a POST request, we don't write anything to the cache. As far as render cache hits go, we check them with their initial cacheability. So by late-placeholdering things on a request where we won't be caching anything anyways I don't see how that would lead to a higher cache hit ratio: It doesn't affect cache gets at all and we're not writing anything to the cache either.

    Tests still go green and MR looks good. Ready to RTBC after this final concern is addressed.

  • 🇬🇧United Kingdom catch

    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.

  • Status changed to RTBC 1 day ago
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    No more notes from my end, looks really good.

Production build https://api.contrib.social 0.61.6-2-g546bc20