Drupal\Core\Extension\ExtensionList::getPathnames() conflates cache and state values

Created on 5 October 2021, over 2 years ago
Updated 30 January 2023, over 1 year ago

Problem/Motivation

Drupal\Core\Extension\ExtensionList::getPathnames() attempts to store and retrieve path names in both cache and state values; however, it does so inconsitently. If the cache value is cleared, then the state value will be returned. Clearing cache will have no affect.

Here's a human-reabable version of the logic:

  • Does a cache value exist?
    • If yes, then return the cache value.
  • Else, does a state value exist?
    • If yes, then return the state value.
    • If no, then recalculate path names
      • Save recalculated path names to cache and state.
      • Return recalculated path names

Here's the code in question:

  /**
   * Returns a list of extension file paths keyed by machine name.
   *
   * @return string[]
   */
  public function getPathnames() {
    if ($this->pathNames === NULL) {
      $cache_id = $this->getPathnamesCacheId();
      if ($cache = $this->cache->get($cache_id)) {
        $path_names = $cache->data;
      }
      // We use $file_names below.
      elseif (!$path_names = $this->state->get($cache_id)) {
        $path_names = $this->recalculatePathnames();
        // Store filenames to allow static::getPathname() to retrieve them
        // without having to rebuild or scan the filesystem.
        $this->state->set($cache_id, $path_names);
        $this->cache->set($cache_id, $path_names);
      }
     $this->pathNames = $path_names;
    }
    return $this->pathNames;
  }

Cache vs state is discussed in #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ where this code originated. See below:

#2208429-62: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by Mile23:

But it should be in the cache, instead. I don't think we have a way to clear state from the UI, and having a filesystem problem and a non-clearable cache is bad news for users.

#2208429-63: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by almaudoh:

Totally agree it should be in cache, not state, but I guess the main consideration was maintaining BC, since we don't know how many contrib modules out there are using \Drupal::state() directly to get module info.

#2208429-70: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by almaudoh:

As a matter of fact, we cannot use State to store extension lists and info because State depends on the system module, and the system module needs module lists for installation -> a circular dependency. So we have no choice but to use cache.

#2208429-88: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by catch:

That's OK to change in a minor version. Module info happening to be in state isn't part of the public API, per https://www.drupal.org/core/d8-bc-policy β†’

#2208429-329: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by catch:

Also what is the situation where the cache will be cleared but the state entry won't be (except for say a memcache eviction)?

#2208429-330: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by dawehner:

Well yes this was the idea. Avoid disc IO as much as possible.

#2208429-332: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by catch:

I'm not sure about this, it means that it will persist past a bin flush. For example what happens if you move a module from modules/custom to modules/contrib?

#2208429-334: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList β†’ by dawehner:

Well, in this case \Drupal\Core\Extension\ExtensionList::reset is called, which removes the state entry. Given that it is totally able to move modules.
Also keep in mind, we do have set('system.module.files', $files); already in core as it is. We are "just" moving it around basically. Does that makes sense?

Unfortunately, \Drupal\Core\Extension\ExtensionList::reset() is not called in all circumstances.

Proposed resolution

Switch to storing exclusively in cache. (I'm not seeing the benefit of the current behavior. How does it reduce IO?)

Remaining tasks

  • Community feedback
  • Open an MR and update tests

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

πŸ› Bug report
Status

Needs work

Version

10.1 ✨

Component
ExtensionΒ  β†’

Last updated 3 days ago

No maintainer
Created by

πŸ‡ΊπŸ‡ΈUnited States Chris Burge

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • The Needs Review Queue Bot β†’ tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

Production build 0.69.0 2024