Problem/Motivation

Coming from #2663638: Avoid checking the render context to detect early rendering for Non-GET requests. and #2729137: NodeStorage::loadByProperties should avoid adding node access query tags.

The check for a render context isn't really sufficient, we can also check isMethodSafe() and skip adding metadata there.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2729211.patch682 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Status: Active » Needs review
FileSize
2729211.patch682 bytes
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Related issues: +#2663638: Avoid checking the render context to detect early rendering for Non-GET requests.
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented

I totally agree that it doesn't make sense to add them here. If we don't render cache, we don't need them anyway here.

Status: Needs review » Needs work

The last submitted patch, 2: 2729211.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Title: node_query_node_acccess_alter() should only add cacheability on GET/HEAD » node_query_node_access_alter() should only add cacheability on GET/HEAD
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Parent issue: » #2638686: Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/node/node.module
@@ -1081,8 +1081,9 @@ function node_query_node_access_alter(AlterableInterface $query) {
   // Bubble the 'user.node_grants:$op' cache context to the current render
   // context.

Let's also update the comment then. Can be done upon commit.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

However … in \Drupal\Core\Render\RenderCache::get() + \Drupal\Core\Render\RenderCache::set():

    // Form submissions rely on the form being built during the POST request,
    // and render caching of forms prevents this from happening.
    // @todo remove the isMethodSafe() check when
    //       https://www.drupal.org/node/2367555 lands.
    if (!$this->requestStack->getCurrentRequest()->isMethodSafe() || !$cid = $this->createCacheID($elements)) {
      return FALSE;
    }

Doing this issue means that solving #2367555: Deprecate EnforcedResponseException support alone will not be enough to allow render cache to be used on POST requests. Which means we won't be able to use render cache for POST requests (i.e. when submitting a form) for things that are not affected until D9. That's probably a fine compromise, but I do want to call it out.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented

If we do #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs then we won't render pages to find forms - this is the most expensive part of form submission. After the form submit we usually redirect, so there'll be no rendering of a page at all - POST -> submit -> RedirectResponse

Logging, e-mail sending etc. might happen as a result of form submission, but we don't need to enable render caching for those.

In irc Wim mention a POST -> submit -> render page flow. I hadn't thought about doing that, but if we did we could revisit all this then. For now I think it's consistent with the overall direction everything's taking.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

+1

RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

If we do #2503429: Allow both AJAX and non-AJAX forms to POST to dedicated URLs then we won't render pages to find forms - this is the most expensive part of form submission. After the form submit we usually redirect, so there'll be no rendering of a page at all - POST -> submit -> RedirectResponse

What about form validation errors?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented

That's a good question. We'd either have to render the form with the validation errors at the dedicated URL we posted to (so render HTML on POST still), or use form storage, redirect back to the form URL itself as a GET request and show the errors there.

In terms of this issue, it only affects whether we do render caching on those post requests (if we have them), not anything else about changing the form submission process. So for me it's fine to change this now, then revisit if we need to when we get to that point.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed cb80d54 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 7bd926f on 8.2.x 
    Issue #2729211 by catch, Wim Leers: node_query_node_access_alter()...

  • alexpott committed cb80d54 on 8.1.x 
    Issue #2729211 by catch, Wim Leers: node_query_node_access_alter()...

Status: Fixed » Closed (fixed)

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