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.