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

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.