DefaultPluginManager should expose its cacheability metadata [#2720813]

Problem/Motivation

DefaultPluginManager caches plugin definitions and supports cache tags out of the box. However, it does not implement any API to expose this information, so external code cannot use this metadata to properly control cache invalidation.

Proposed resolution

Make DefaultPluginManager implement CacheableDependencyInterface.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#6	drupal_2720813_6.patch	3.42 KB	Xano
#6	8.1.x: PHP 5.5 & MySQL 5.5 18,592 pass PHP 5.6 & MySQL 5.5 18,592 pass PHP 7 & MySQL 5.5 18,582 pass
#6	interdiff.txt	752 bytes	Xano
#2	drupal_2720813_2.patch	4.16 KB	Xano
#2	8.2.x: PHP 5.5 & MySQL 5.5 18,523 pass, 1 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

7 May 2016 at 15:23

Xano created an issue. See original summary.

Comment #2

Xano

he/they

English

Southampton

CreditAttribution: Xano at Druid commented 7 May 2016 at 15:24

Status:

Active

» Needs review

File	Size
drupal_2720813_2.patch	4.16 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,523 pass, 1 fail

Comment #3

Xano

he/they

English

Southampton

CreditAttribution: Xano at Druid commented 7 May 2016 at 15:25

Issue summary:

View changes

Comment #4

7 May 2016 at 16:11

Status:

Needs review

» Needs work

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

Comment #5

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 7 May 2016 at 16:16

Issue tags:

+D8 cacheability

Comment #6

Xano

he/they

English

Southampton

CreditAttribution: Xano at Druid commented 7 May 2016 at 17:52

Status:

Needs work

» Needs review

File	Size
interdiff.txt	752 bytes
drupal_2720813_6.patch	3.42 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,592 pass PHP 5.6 & MySQL 5.5 18,592 pass PHP 7 & MySQL 5.5 18,582 pass

1 file was hidden/shown/deleted

File	Size
drupal_2720813_2.patch	4.16 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,523 pass, 1 fail

Too bad we can't document this on the interface. Any suggestions for where else this would fit?

Comment #7

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 7 May 2016 at 23:48

So what I'm wondering, does this mean we have issues in existing core which should use that?

Comment #8

Xano

he/they

English

Southampton

CreditAttribution: Xano at Druid commented 8 May 2016 at 01:20

Yes. Anything based on plugin definitions has no cacheability metadata associated with it, except cached plugin definitions which in many cases use cache tags, but those are internal to plugin managers and not exposed to other code. This patch builds on the existing cache tag functionality to expose it.

This is important for derivative plugins, for instance. Their definitions don't just change when files are updated, but also as a result of CRUD operations on the config entities from which those definitions are derived.

I wonder if we should add Drupal\Core\Plugin\Discovery\CachedDiscoveryInterface extends Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface, Drupal\Core\Cache\CacheableDependencyInterface to clearly indicate that if you cache definitions, you'd better tell other code how to interact with those caches.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 14 June 2016 at 14:37

Status:

Needs review

» Reviewed & tested by the community

+1 for the principle.

The code looks great.

+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
@@ -301,4 +302,40 @@ public function testGetDefinitionsWithoutRequiredInterface() {
+    $this->assertInternalType('array', $cache_contexts);
...
+      $this->assertInternalType('string', $cache_context);
...
+    $this->assertInternalType('array', $cache_tags);
...
+      $this->assertInternalType('string', $cache_tag);
...
+    $this->assertInternalType('int', $cache_max_age);

Cool, I didn't know this PHPUnit assertion!

Comment #10

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 14 June 2016 at 14:48

Status:

Reviewed & tested by the community

» Needs review

Is there a concrete use case for core that could have a follow-up issue?

Comment #11

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 14 June 2016 at 18:05

Status:

Needs review

» Reviewed & tested by the community

Yes, there are plenty:

where the 'entity_types' cache tag is currently being added manually, it should use EntityTypeManager::getCacheTags()
… 'entity_field_info' … EntityFieldManager::getCacheTags()
… 'breakpoints' … BreakpointManager::getCacheTags()
… 'contextual_links_plugins' … ContextualLinkManager::getCacheTags()
… 'local_action' … LocalActionManager::getCacheTags()
… 'local_task' … LocalTaskManager::getCacheTags()

et cetera. And in fact, any plugin manager that doesn't yet specify a cache tag is kinda buggy in that respect, because AFAICT there's no reason for it not to have cache tags. Which means that anything depending on those plugins currently has incomplete cacheability metadata.

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 15 June 2016 at 13:04

Version:	8.1.x-dev	» 8.2.x-dev
Status:	Reviewed & tested by the community	» Fixed

As this is an API addition I think we shouldn't add this to 8.1.x. If I'm wrong and this bug should be fixed in 8.1.x then feel free to re-open.

Committed 78bb26d and pushed to 8.2.x. Thanks!

Comment #13

15 June 2016 at 13:04

alexpott committed 78bb26d on 8.2.x

Issue #2720813 by Xano, Wim Leers: DefaultPluginManager should expose...

Comment #14

Xano

he/they

English

Southampton

CreditAttribution: Xano at Druid commented 15 June 2016 at 13:16

Thanks!

I wouldn't say this is an API issue, because it only extends a class to implement an API. I'm not against leaving this out of 8.1.x, although including it will help with performance improvements. The Verf and Plugin modules are able to use this new cacheability metadata already.