Spin-off to prepare for #2155635: Allow plugin managers to opt in to cache clear during module install

Problem

  • The static + persistent cache of FieldInfo is not cleared when necessary; e.g., when installing a new module.

Details

  • FieldInfo is an aggregate cache of plugin definitions and configuration data.
  • It cannot implement the existing CachedDiscoveryInterface of the Plugin component, because that represents a limited and different (discovery) concept only.

Proposed solution

  1. Introduce a new Drupal\Core\State\CachedStateInterface.

    To be implemented by "manager" classes that are statically/persistently caching state information, which only needs to be rebuilt upon explicit request; i.e., when the application knows that data has changed.

  2. Make FieldInfo implement it.
  3. Rename the existing flush() method into clearCachedState()

  4. → Allow us to clear the cached state of all corresponding services when e.g. installing/uninstalling a module.

    The meat of that is left to #2155635: Allow plugin managers to opt in to cache clear during module install

Notes

In a separate follow-up issue, it would probably be a good idea to revisit the router specific changes from #2164367: Rebuild router as few times as possible per request and move those setRebuildNeeded() + rebuildIfNeeded() + etc methods into this new CachedStateInterface, since that would ultimately give us:

  1. Enforced state cache clearing.
  2. On-demand rebuilding upon next access (or end-of-request otherwise).
  3. A single shared interface for such cached state services (that can be collected through a CompilerPass).

While grepping through core for FieldInfo usages, I noticed that EntityManager has both a clearCachedDefinitions() and a separate clearCachedFieldDefinitions() method, whereas the former does not call into the latter. That is probably a separate "bug" that needs to be fixed as well (but not here).

CommentFileSizeAuthor
#11 interdiff.txt1.18 KBsun
#11 state.cache_.11.patch6.01 KBsun
#8 interdiff.txt5.72 KBsun
#8 state.cache_.8.patch7.2 KBsun
#5 drupal8.field-info-clear.4.patch6.57 KBsun
#3 drupal8.field-info-clear.3.patch5.64 KBsun
drupal8.field-info-clear.0.patch5.24 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, drupal8.field-info-clear.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

eeeek! :-/

interface CachedDiscoveryInterface extends DiscoveryInterface {
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.field-info-clear.3.patch5.64 KB

I already assume there will be reservations to this solution/workaround... ;)

Status: Needs review » Needs work

The last submitted patch, 3: drupal8.field-info-clear.3.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.field-info-clear.4.patch6.57 KB

Now, the much more appropriate resolution is to decouple the CachedDiscoveryInterface from DiscoveryInterface.

Since PluginManagerInterface already implements DiscoveryInterface, almost no changes are required.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

If we do this, then we need to:
a) Move it out of the Plugin and Discovery namespaces
b) Remove the word "Discovery"
c) Rework #2155635: Allow plugin managers to opt in to cache clear during module install to be about "things that have statically or persistently cached definitions and need them cleared" and not specific to plugin managers at all

We cannot just borrow the interface like this, and masquerade FieldInfo as something related to either plugins or discovery, since it is neither.

I discussed this with @EclipseGc and @neclimdul, and they concurred.

Status: Needs review » Needs work

The last submitted patch, 5: drupal8.field-info-clear.4.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new state.cache_.8.patch7.2 KB
new interdiff.txt5.72 KB

After some longer discussion in IRC, it appears to be best to leave the CachedDiscoveryInterface concept of the (decoupled) Plugin component alone.

Instead, we introduce a separate/standalone interface for manager classes that are statically/persistently caching state information, which only needs to be rebuilt upon explicit request; i.e., when the application knows that data has changed.

Attached patch adds a new Drupal\Core\State\CachedStateInterface.

In a separate follow-up issue, it would probably be a good idea to revisit the router specific changes from #2164367: Rebuild router as few times as possible per request and move those setRebuildNeeded() + rebuildIfNeeded() + etc methods into this new CachedStateInterface, since that would ultimately give us:

  1. Enforced state cache clearing.
  2. On-demand rebuilding upon next access (or end-of-request otherwise).
  3. A single shared interface for such cached state services (that can be collected through a CompilerPass).
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Also created #2202629: Move Drupal\Core\KeyValueStore\State* into Drupal\Core\State\State*

neclimdul’s picture

neclimdul
He/Him
commented 
+++ b/core/lib/Drupal/Component/Plugin/Discovery/CachedDiscoveryInterface.php
@@ -8,16 +8,16 @@
-   * Don't resort to calling \Drupal::cache()->delete() and friends to make
-   * Drupal detect new or updated plugin definitions. Always use this method on
-   * the appropriate plugin type's plugin manager!
+   * Do not try to manually clear the cache of plugin managers implementing this
+   * interface to detect new or updated plugin definitions. Always use this
+   * method on the appropriate plugin type's plugin manager instead.
    */

The old docs are not really about Plugins which is problematic for Drupal\Component so that is a problem. However I'm not sure the new docs really describe the method well outside of the context of this issue.

Rather then holding up this issue on unrelated documentation lets discuss it separately.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new state.cache_.11.patch6.01 KB
new interdiff.txt1.18 KB

Sure. → Reverted all changes to CachedDiscoveryInterface.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Make FieldInfo implement CachedDiscoveryInterface » Make FieldInfo implement a new CachedStateInterface
Issue summary: View changes

Adjusting issue title + summary accordingly.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

As commented in the other issue, field info caches are not the problem. I don't see how it helps us for #2201785: Remove drupal_flush_all_caches() from installer. Field types are.

FieldInfo is also slated to be removed and merged with the EntityManager, see #2116363: Unified repository of field definitions (cache + API). Which would make it part of clearCachedDefinitions() if we want to without requiring any further API changes.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Closed (won't fix)

FieldInfo is no more. All field caches are cleared as part of clearCachedDefinitions() on EntityManager.