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

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

catch created an issue. See original summary.

catch’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
871 bytes

Untested patch.

Status: Needs review » Needs work

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

catch’s picture

catch’s picture

catch’s picture

Status: Needs work » Needs review
dawehner’s picture

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

#7++

catch’s picture

Status: Needs work » Needs review
FileSize
2.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

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.

tom_ek’s picture

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.

tom_ek’s picture

Status: Needs work » Needs review

Rerunning the tests.

tom_ek’s picture

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

Status: Needs review » Reviewed & tested by the community

Good catch @tom_ek!

larowlan’s picture

  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

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.