\Drupal\Core\DefaultContent\Finder::__construct() builds the dependency graph keyed by UUID, sorts it with \Drupal\Component\Graph\Graph::searchAndSort(), and reduces it to an ordered $this->data — with no check for dependency cycles. When _meta.depends forms a cycle (e.g. A→B→C→A), no valid topological order exists, but searchAndSort() still returns an arbitrary order, so at least one entity is imported before a dependency it references.
In \Drupal\Core\DefaultContent\Importer, setFieldValues() then calls loadEntityDependency(), which returns NULL because the unresolved target isn't yet imported; that NULL is passed straight to $property->setValue(). (Method line numbers vary by branch; verified on the current dev tip.)
entity_reference property this silently stores an empty reference — data loss whose occurrence depends on the filesystem iteration order of the Symfony Finder scan, so a recipe can pass CI on one machine and silently lose different data on another.entity_reference_revisions it is fatal, aborting drush site:install.There is no NULL-guard in loadEntityDependency() and no cycle detection upstream. Silent, machine-dependent data loss is the worst outcome here; failing loudly at import time surfaces the authoring mistake immediately. (Filing as Normal, but flagging the data-loss angle for a priority call.)
Author default-content YAML whose _meta.depends declarations form a cycle (e.g. a node → paragraph (ERR field_sections) → node → first node), then drush site:install <recipe>. Observed on a service node → paragraph (ERR) → node → service cycle.
Detect cycles in Finder immediately after Graph::searchAndSort() and throw ImportException naming the offending source files, instead of returning a broken order. A cycle is detected by self-reachability (isset($vertex['paths'][$uuid])); the full member set of each cycle is the strongly-connected component paths ∩ reverse_paths (so the back-edge vertex is named too, not just the entry vertices).
Tests included:
FinderTest): three-node cycle, self-loop, and two disjoint cycles — asserts every file in every cycle is named.ImporterTest): a cyclic directory makes importContent() throw before writing anything (fail-before-write). Verified RED→GREEN at both levels; no regressions across DefaultContent unit + kernel tests; PHPCS (Drupal/DrupalPractice) clean.Importer::loadEntityDependency() would be complementary defense-in-depth, but it can only see one unresolved UUID mid-import and can't name the cycle. Proposing the Finder fix as fix-of-record, with the guard as optional follow-up hardening.An MR will follow from an issue fork (patch + unit/kernel tests are ready).
Created with the help of Claude Fable
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
alex ua commentedComment #4
quietone commented@alex ua, Thanks for the disclosure of AI usage.
Issues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.
Also restoring issue template so we can track progress and task.
Comment #5
phenaproximaI am of two minds on this.
On the one hand: it's a legitimate bug and has thorough test coverage (which I have not reviewed in depth).
On the other hand, this is not really a problem with the default content system per se. Consider the doc comment of
\Drupal\Component\Graph\Graph, which says:To me, this says that the
Graphclass is assuming there are no cycles in the graph, but not verifying it, which leads to the problems surfaced in this issue. So the flaw is lower-level than the default content system.IMHO, it should not be possible to feed a cyclic graph to the
Graphclass. It should fail with an exception. But that would be a BC break.So I propose the following:
Graphclass.verifyGraphIsAcyclic(). It should throw an exception if the graph has any cycles, with a clear outline of the cyclic path to aid in debugging bad input data.searchAndSort()is called without having first calledverifyGraphIsAcyclic(), raise a deprecation notice. This means all calling code needs to confirm it's providing an acyclic graph in the first place, and handle that case as needed.searchAndSort()should always callverifyGraphIsAcyclic(), and we remove the deprecation.Thoughts?
Comment #6
phenaproximaIt occurs to me that we might not be able to tell if the graph has cycles before we search-and-sort it. But even so, we should raise an error: when
searchAndSort()is called, as a last step before returning it should check for cycles. If it finds one, it raises a deprecation notice. In Drupal 13, we convert that to an exception.Comment #7
alex ua commentedThanks, that direction makes sense to me.
I reworked the patch around Graph rather than keeping the cycle detection only in DefaultContent:
- Added Drupal\Component\Graph\Graph::verifyGraphIsAcyclic().
- Added Drupal\Component\Graph\CyclicGraphException with a concrete cycle path in the message.
- Added a deprecation from Graph::searchAndSort() when it is called on a cyclic graph without prior acyclic verification, with the Drupal 13 path left as the hard exception.
- Updated DefaultContent Finder to call the verifier and wrap the graph exception as an ImportException that names the default-content source files involved in the cycle.
- Kept the DefaultContent fail-before-write kernel coverage, so recipe authors get the actionable import failure rather than a later NULL reference or fatal.
That preserves the reported DefaultContent behavior while moving the invariant to the lower-level graph API. I am not claiming every existing Graph caller can safely throw in 11.x; the deprecation path is there for that BC boundary.
Prepared with AI assistance.