Problem/Motivation

Same thing as #2729211: node_query_node_access_alter() should only add cacheability on GET/HEAD.

In this case a URL was being generated for logging, but toString() was called without setting $collect_bubblable_metadata to FALSE, resulting in our favourite exception message.

Proposed resolution

Only collect bubbleable metadata if both the argument is set, and the method is safe. This is a behaviour change of course.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 interdiff-2744715.txt3.17 KBtrzcinski.t@gmail.com
#18 2744715-18.patch5.09 KBtrzcinski.t@gmail.com
#16 interdiff-2744715.txt2.35 KBtrzcinski.t@gmail.com
#16 2744715-16.patch5.09 KBtrzcinski.t@gmail.com
#12 2744715.patch2.74 KBcatch
#10 2744715.patch2.52 KBcatch
#5 2744715.patch839 bytescatch
#2 2744715.patch871 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
Priority: Normal » Major
Status: Active » Needs review
FileSize
2744715.patch871 bytes

Untested patch.

Status: Needs review » Needs work

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

catch’s picture

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

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
FileSize
2744715.patch839 bytes
catch’s picture

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

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Can we add this check in \Drupal\Core\Render\MetadataBubblingUrlGenerator::bubble instead? That seems to be the better place to be honest

Status: Needs review » Needs work

The last submitted patch, 5: 2744715.patch, failed testing.

Wim Leers’s picture

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

#7++

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Status: Needs work » Needs review
FileSize
2744715.patch2.52 KB

Yes that's a lot better. No interdiff since completely different patch.

Status: Needs review » Needs work

The last submitted patch, 10: 2744715.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Tag1 Consulting commented
Status: Needs work » Needs review
FileSize
2744715.patch2.74 KB

Missing use statement.

Status: Needs review » Needs work

The last submitted patch, 12: 2744715.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com as a volunteer commented
FileSize
2744715-16.patch5.09 KB
interdiff-2744715.txt2.35 KB

Hi. This patch still applies cleanly (checked against 8.4.x). Works great!

I have made the tests pass and added a safe / non-safe test cases (not sure if this is correct approach though).

Attaching the improved patch and the interdiff.

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com as a volunteer commented
Status: Needs work » Needs review

Rerunning the tests.

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com as a volunteer commented
FileSize
2744715-18.patch5.09 KB
interdiff-2744715.txt3.17 KB

Seems like he isMethodSafe is deprecated in this context? I have changed it to isMethodCacheable which sounds like a real way to go. Interdiff attached again (with the @catch'es patch)

dawehner’s picture

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

Good catch @tom_ek!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
  1. +++ b/core/core.services.yml
index 6a5527b..d1327d1 100644
--- a/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php

+++ b/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php
@@ -35,16 +36,27 @@ class MetadataBubblingUrlGenerator implements UrlGeneratorInterface {
+
+

    nit: two blank lines - can be fixed on commit 

  2. +++ b/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php
@@ -4,6 +4,7 @@
@@ -35,16 +36,27 @@ class MetadataBubblingUrlGenerator implements UrlGeneratorInterface {

@@ -35,16 +36,27 @@ class MetadataBubblingUrlGenerator implements UrlGeneratorInterface {
+  protected $requestStack;

    Was there a reason that MetadataBubblingUrlGenerator decorated the base generator instead of extending it?

    setContext, getContext, getPathFromRoute, supports and getRouteDebugMessage all defer to decorated, but could easily defer to parent. Then we could remove those methods.

    generate and generateFromRoute add the bubble info, but could easily call parent::{method} instead of $this->urlGenerator.

    If we did that, we'd have the request stack for free, because the base generator already has it injected.

    Just a thought - could be a reason why we did it the way we did.

  3. +++ b/core/tests/Drupal/Tests/Core/Render/MetadataBubblingUrlGeneratorTest.php
@@ -50,9 +51,13 @@ protected function setUp() {
-  public function testUrlBubbleableMetadataBubbling($collect_bubbleable_metadata, $invocations, array $options) {
+  public function testUrlBubbleableMetadataBubbling($collect_bubbleable_metadata, $invocations, $method, array $options) {

    is there a reason why we changed the order of arguments here?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs work
Related issues: +#2367555: Deprecate EnforcedResponseException support, +#2653998: Allow to opt-in for render caching on POST requests 
+++ b/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php
@@ -80,8 +92,10 @@ protected function bubble(GeneratedUrl $generated_url, array $options = []) {
+    // response cacheability. Only do this for GET/HEAD requests since render
+    // caching does not apply to other responses.

Note that this is being changed by #2367555: Deprecate EnforcedResponseException support + #2653998: Allow to opt-in for render caching on POST requests. Render caching will work for e.g. POST requests then. Because thanks to cache tags, we will know which parts of the page will have been invalidated. So using render caching will no longer need to be disabled for POST requests.

At minimum, this needs to reference those issues.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.