Problem/Motivation
Config dependencies are checked for existence. They are not checked for correctness.
What core checks today
#3324150: Add validation constraints to config_entity.dependencies added constraints to
config_dependencies_base in core/config/schema/core.data_types.schema.yml:
NotBlankandValidKeysConfigExistson eachconfigdependencyExtensionNameandExtensionExistson eachmoduleand
themedependency
Together these answer one question: does each listed dependency exist?
What core does not check
Whether the listed dependencies are the ones core itself would calculate. Nothing compares stored
dependencies to ConfigEntityBase::calculateDependencies(). Grep for
calculateDependencies in Core/Config/Plugin/Validation/Constraint and
Core/Validation/Plugin/Validation/Constraint. There are no hits.
Why that matters
ConfigEntityBase::preSave() recalculates dependencies on every save. It skips this
during a sync:
if (!$this->isSyncing()) {
// Ensure the correct dependencies are present. If the configuration is
// being written during a configuration synchronization then there is no
// need to recalculate the dependencies.
$this->calculateDependencies();
So a config import stores whatever the source says. Then, on module uninstall,
ConfigManager::uninstall() deletes every config entity it cannot repair:
foreach ($entities['delete'] as $entity) {
$entity->setUninstalling(TRUE);
$entity->delete();
}
ConfigEntityBase::onDependencyRemoval() cannot repair a plain module dependency. It
returns FALSE, so the config entity is deleted. One wrong line therefore destroys
content:
- Add
dependencies.module: [foo]tonode.type.article. - Uninstall
foo. - The content type is deleted. Its nodes go with it.
Where this can happen
Anywhere config reaches a site from outside it:
- a Git-backed config workflow
- a recipe
- a hand-edited sync directory
Steps to reproduce
- Add
dependencies: {module: [views]}tonode.type.articlein the sync
directory. - Run
drush config:import. It is accepted: sync skips recalculation, and
viewsexists. - Uninstall
views. The content type and its nodes are deleted.
Proposed resolution
Add a CalculatedConfigDependencies constraint. It compares stored dependencies to
calculated ones and reports a violation per difference, in both directions.
The name follows the existing RequiredConfigDependencies in that namespace, which
checks that dependencies contain given entity types. This one checks that they are the dependencies
calculateDependencies() produces. (ValidConfigDependencies would be a poor name, given the pre-existing validation that #3324150: Add validation constraints to config_entity.dependencies added.
Exclude enforced from the comparison
An enforced dependency is not calculated, so it must be left out of both sides. There is a trap
here. ConfigEntityBase::getDependencies() merges enforced into what it
returns:
$dependencies = $this->dependencies;
if (isset($dependencies['enforced'])) {
// Merge the enforced dependencies into the list of dependencies.
$enforced_dependencies = $dependencies['enforced'];
unset($dependencies['enforced']);
$dependencies = NestedArray::mergeDeep($dependencies, $enforced_dependencies);
}
So removing enforced after calling that method is too late. An enforced entry then
reads back as a calculated one. Remove it from the stored values before calculating instead.
What makes this hard
We built this in a distribution. One problem needs solving first:
calculateDependencies() cannot always run before a change set is applied. Some
implementations read other config, which the same set may still be about to create.
The main case is a bundle. EntityTypeInterface::getBundleConfigDependency() loads
the bundle entity and throws a LogicException when it is missing, and four
calculateDependencies() implementations call it:
| Caller | Config entity types affected |
|---|---|
FieldConfigBase::calculateDependencies() |
field_config, base_field_override |
EntityDisplayBase::calculateDependencies() |
entity_form_display, entity_view_display |
ContentModeration::calculateDependencies() |
workflow |
ContentLanguageSettings::calculateDependencies() |
language_content_settings |
So any config entity whose dependencies include a bundle cannot be calculated while that bundle
is still in the change set. Types that read no other config —
user_role, node_type, field_storage_config — can.
Two implementations fail in a second, worse way, by returning too few dependencies instead of
throwing:
FieldConfig::calculateDependencies()also calls
getFieldStorageDefinition(), which throws when the field storage is in the same
set.EntityDisplayBase::calculateDependencies()intersects components with
getFieldDefinitions()and silently returns too few.
EntityFormDisplay::getPluginCollections()also skips components whose field it
cannot resolve, so widgetmoduledependencies are lost too.- Field item implementations can do the same:
EntityReferenceItemresolves
target_bundleswithloadMultiple()and skips the ones absent.
Returning too few is the dangerous case. A real dependency then looks unexpected, so a naive
constraint rejects valid imports. The common one is adding a content type, its fields and its form
display together. Throwing is safer: a caller can treat it as "cannot say" and check nothing.
Out of scope: whether an enforced dependency is correct
dependencies.enforced exists so that a module can declare a dependency core cannot
calculate. Core therefore cannot tell a legitimate enforced dependency from a wrong one: it cannot
know whether some shipped configuration once declared it. Only the owner of a config workflow can
decide that. A distribution that syncs customer-authored configuration knows an enforced dependency
can never be legitimate there, and can refuse it. Core has no such rule available.
This is worth noting because an enforced dependency is the one kind nothing ever corrects.
calculateDependencies() keeps the key on every save, and
ConfigEntityDependency::__construct() merges it into the dependency graph, so Drupal acts
on it.
Remaining tasks
- Decide where the comparison lives: an entity-level constraint on
ConfigEntityType,
or a constraint on theconfig_dependenciesschema type. - Decide what to do for the types that cannot be calculated before apply.
- Decide whether this is opt-in. Existing sites may hold drift that would now fail.
- Tests.
User interface changes
None.
API changes
A new validation constraint is added.
Data model changes
None.
Comments
Comment #2
wim leersComment #3
wim leers