Problem/Motivation

      if ($url->isRouted()) {
        $allowed = TRUE;
        try {
          $url->toString();
        }
        // The following exceptions are all possible during URL generation, and
        // should be considered as disallowed URLs.
        catch (RouteNotFoundException $e) {
          $allowed = FALSE;
        }
        catch (InvalidParameterException $e) {
          $allowed = FALSE;
        }
        catch (MissingMandatoryParametersException $e) {
          $allowed = FALSE;
        }
        if (!$allowed) {
          $this->context->addViolation($constraint->message, ['@uri' => $value->uri]);
        }
      }

Note the $url->toString(); in there; this bubbles cacheability. But for the purposes of this validation, that's pointless and meaningless. It should not bubble cacheability.

Proposed resolution

Collect cacheability instead of allowing it to be bubbled.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#16 2982060-16.patch1.26 KBWim Leers
#3 2982060-3.patch933 bytesWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2948666: Remove JSON API's use of $context['cacheable_metadata']

Discovered at #2948666-37: Remove JSON API's use of $context['cacheable_metadata'].

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
2982060-3.patch933 bytes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Contributed project blocker
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: LinkNotExistingInternalConstraintValidator is bubbling cacheability » LinkNotExistingInternalConstraintValidator is bubbling cacheability (and shouldn't)

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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Minor » Normal
Related issues: +#2991135: Create entity - internal link - The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse.

Looks like this same bug was reported again: #2991135: Create entity - internal link - The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\\jsonapi\\ResourceResponse.. Bumping from Minor to Normal.

Wim Leers’s picture

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

@sorina.hriban confirmed that this patch fixed his problem reported in #2991135!

sorina.hriban’s picture

sorina.hriban CreditAttribution: sorina.hriban commented

I confirm also here that the patch added resolved the link internal problem.

sorina.hriban’s picture

sorina.hriban CreditAttribution: sorina.hriban commented
Status: Needs review » Reviewed & tested by the community
Wim Leers’s picture

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

Yay, thanks, @sorina.hriban!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Can we add something to LinkNotExistingInternalConstraintValidatorTest to ensure nothing is bubbled?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review
Issue tags: +API-First Initiative

I don't see how we can do that without creating a new functional test. Because the current test is a unit test (as it should be), and hence we'd need to mock both toString() and toString(TRUE) behavior, and then fail in case of toString(). IOW: it'd require mocking of things outside the unit…

The JSON API module does have functional tests that ran into this problem: \Drupal\Tests\jsonapi\Functional\ShortcutTest::testPostIndividual() are marked as skipped because of this bug.

Is it really worth creating a full functional test for this tiny trivial single-line patch?

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.

psf_’s picture

psf_ CreditAttribution: psf_ at SDOS commented

Drupal 8.7.5 has this patch applied, or have the same modification in it. ¯\@¿@/¯

But I have the same problem with JSON:API, the same query with filter fail and without filter not.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: LinkNotExistingInternalConstraintValidator is bubbling cacheability (and shouldn't) » Un-skip JSON:API tests because LinkNotExistingInternalConstraintValidator is no longer incorrectly bubbling cacheability
Component: link.module » jsonapi.module
Category: Bug report » Task
Issue tags: -Contributed project blocker, -Needs tests
FileSize
2982060-16.patch1.26 KB

Wow. Mindbending.

#13 and #15 are exactly one year apart. I looked at the date of #13 and thought I had forgotten part of my memory 🤯

Reading #13 … I now have a different answer: the test I mentioned there is now part of Drupal core, thanks to JSON:API landing in core.

So I tried to unmark those tests as skipped, so they should fail again:

  /**
   * {@inheritdoc}
   */
  public function testPostIndividual() {
    $this->markTestSkipped('Disabled until https://www.drupal.org/project/drupal/issues/2982060 is fixed.');
  }

  /**
   * {@inheritdoc}
   */
  public function testRelationships() {
    $this->markTestSkipped('Disabled until https://www.drupal.org/project/drupal/issues/2982060 is fixed.');
  }

  /**
   * {@inheritdoc}
   */
  public function testPatchIndividual() {
    $this->markTestSkipped('Disabled until https://www.drupal.org/project/drupal/issues/2982060 is fixed.');
  }

… but turns out the fix (the patch in #3) already landed as part of #2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Un-skip JSON:API tests because LinkNotExistingInternalConstraintValidator is no longer incorrectly bubbling cacheability » Un-skip skipped JSON:API tests because LinkNotExistingInternalConstraintValidator is no longer incorrectly bubbling cacheability
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Acquia commented
Status: Needs review » Reviewed & tested by the community

Un-skipping tests, tests still pass, reason given for why they pass now but didn't pass before checks out... LGTM!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed d6bc614 and pushed to 8.8.x. Thanks!

  • larowlan committed d6bc614 on 8.8.x 
    Issue #2982060 by Wim Leers: Un-skip skipped JSON:API tests because...

Status: Fixed » Closed (fixed)

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