blocker to #2335661: Outbound path & route processors must specify cacheability metadata, based on discussion wiht Wim Leers

Problem/Motivation

For menu links or any generated URL strings we need to track cache metadata.

The current BubbleableMetadata is too specific to render arrays and handles attached assets, etc. We don't need the extra overhead when handling a simple string.

Proposed resolution

Create a superclass of BubbleableMetadata then omits logic not needed for simpler data types. Make the current BubbleableMetadata as subclass of it, possibly renaming it.

Remaining tasks

User interface changes

n/a

API changes

Class name change.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Wim Leers’s picture

Either CacheableMetadata or CacheabilityMetadata are the names that come to mind.

pwolanin’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
15.98 KB
pwolanin’s picture

FileSize
6.71 KB

whoops - that had some other crap in it. Here's the right patch.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
    @@ -0,0 +1,105 @@
    + * Defines an interface for objects which may be used by other cached objects.
    + *
    + * This extended interface defines setters so that the object may have cache
    

    This is not an interface, but a class :)

  2. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadataInterface.php
    @@ -0,0 +1,73 @@
    +interface CacheableMetadataInterface extends CacheableDependencyInterface {
    

    Why is this interface necessary? Why would anyone ever want to swap out the CacheableMetadata class?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.46 KB
4.27 KB

ok.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +blocker

Blocks #2335661: Outbound path & route processors must specify cacheability metadata.

Patch looks great; now just a matter of moving a bunch of BubbleableMetadataTest to CacheableMetadataTest.

+++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
@@ -7,14 +7,11 @@
+ * Defines a generic class for storing and fetching cacheability metadata.

s/storing and fetching/passing/

But that's a nit.

The last submitted patch, 3: 2471743-2.patch, failed testing.

The last submitted patch, 6: 2471743-6.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.56 KB
2.77 KB

Cache::mergeContexts() calls out to a service, so can't unit test it.

Status: Needs review » Needs work

The last submitted patch, 10: 2471743-9.patch, failed testing.

Wim Leers’s picture

Issue tags: -Needs tests +Novice, +Needs reroll

Failing tests :(

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php
@@ -0,0 +1,72 @@
+  public function testAddCacheTags() {
+    $metadata = new CacheableMetadata();
+    $add_expected = [
+      [ [], [] ],
+      [ ['foo:bar'], ['foo:bar'] ],
+      [ ['foo:baz'], ['foo:bar', 'foo:baz'] ],
+      [ ['axx:first', 'foo:baz'], ['axx:first', 'foo:bar', 'foo:baz'] ],
+      [ [], ['axx:first', 'foo:bar', 'foo:baz'] ],
+      [ ['axx:first'], ['axx:first', 'foo:bar', 'foo:baz'] ],
+    ];
+
+    foreach ($add_expected as $data) {
+      list($add, $expected) = $data;
+      $metadata->addCacheTags($add);
+      $this->assertEquals($expected, $metadata->getCacheTags());
+    }
+  }

Could use a data provider.

pwolanin’s picture

@Wim Leers - no, I considered a data provider, and I don't think it makes sense because I'm accumulating tags - in other words, the correctness of each expected result depends on the prior ones. I don't think that's how data providers usually work.

pwolanin queued 10: 2471743-9.patch for re-testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Novice, -Needs reroll

Oh, sorry, missed that.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed a0ee087 and pushed to 8.0.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php b/core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php
index a292e9d..7918d60 100644
--- a/core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php
+++ b/core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php
@@ -44,7 +44,7 @@ public function testAddCacheTags() {
    * Test valid and invalid values as max age.
    *
    * @covers ::setCacheMaxAge
-   * @dataProvider providersetCacheMaxAge
+   * @dataProvider providerSetCacheMaxAge
    */
   public function testSetCacheMaxAge($data, $expect_exception) {
     $metadata = new CacheableMetadata();
@@ -58,7 +58,7 @@ public function testSetCacheMaxAge($data, $expect_exception) {
   /**
    * Data provider for testSetCacheMaxAge.
    */
-  public function providersetCacheMaxAge() {
+  public function providerSetCacheMaxAge() {
    return [
      [0 , FALSE],
      ['http', TRUE],

Fixed a capitalisation thing on commit.

  • alexpott committed a0ee087 on 8.0.x
    Issue #2471743 by pwolanin: Create a more generic superclass of  \Drupal...
Fabianx’s picture

+1 to this change!

Status: Fixed » Closed (fixed)

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