#3555413: Allow linking to referenced entities: add `url` property to `EntityReferenceItem::propertyDefinitions()` introduced a new computed data, with type computed_entity_canonical_relative_url.
It accesses the "entity" property of entity references and asserts
\assert($referenced_entity_adapter instanceof EntityAdapter);
after checking
($field_item->isEmpty())
However, that's not a safe assertion. Non empty field-items can have a NULL $referenced_entity_adapter when the entity reference fails to load, e.g. it got deleted meanwhile or it's another special case like a user reference referencing the anonymous user.
I ran into this with custom_elements test coverage, which tried to access the property, and broke for the user-reference to anonymous users. Also see #3556412: 3.x-dev canvas integration test fails of today.
Check for NULL entity adapters.
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
fagoComment #4
fagoMR is green besides some cypress E2E fails, which seem to be unrelated/flaky? tests.
Comment #5
penyaskitoNetwork issue on the testrunners, requeued. LGTM, would like a second review by Wim.
Comment #7
wim leersThis introduces 2 regressions AFAICT, 1 of which can be mitigated, 1 of which is unavoidable and really caused by a decades-old bug: #1368386: Delete references to deleted entities.
I tried to strike a more reliable middle ground. WDYT?
Comment #8
wim leersComment #9
fagohm, not sure what regressions are caused by this, but there shouldn't be any.
As you know, entity references in Drupal can be, by design, stale.
It's up to every consuming code to deal with it correctly. So do we need to do here.
I cannot follow why/what filtering out stale references broke here, but if someone breaks I'd assume this is just the indication of a problem, not the problem per se. For example, cache-metadata needs to be place so that caches are correctly invalidated when the referenced entity is deleted. But I cannot really follow what the proposed middle-ground is trying to do, could you clarify?
Comment #11
wim leersIndeed.
And I'm saying that precisely that is what was missing. Specifically:
is the problem that https://git.drupalcode.org/project/canvas/-/merge_requests/300/diffs?com... aimed to fix:
If the referenced entity fails to load because it is missing, that might be caused either by an actual deletion (which would indeed refer to a stale reference, for which #1368386: Delete references to deleted entities is the core issue to fix), or by a temporary state that could occur e.g. while deploying code/executing scripts/applying a recipe (which should happen for recipe applying since #3442022 but given its ancestry, is likely to be incomplete, see #3442022-17: Trigger entity validation in \Drupal\Core\DefaultContent\Importer::importContent()).
This could for example occur when the referenced entity is not yet saved, because
\Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidatoralso allows referencing new (unsaved) entities.So yes, this is me trying to think about all edge cases and be extra cautious in Canvas since it depends on a super complex tree of dependencies. I've been bitten too many times by the content, config, migration and recipe systems not performing the validation one would reasonably expect 😨
Comment #12
penyaskito⬆️ #11: I was thinking how that temporary state scenario could actually happen, when ids aren't reused in content entities. But it's even easier: the trash module swaps the entity storage manager, so this is actually a problem very easy to reproduce and would be an issue with Drupal CMS.
I didn't manually test it, but reading the middle-ground code it makes sense to me.
Comment #13
fagoI see, that makes completely sense, thanks for the explanation!
As commented on the MR I'm not sure throwing an exception is a good idea though. Maybe we can prevent the potential issue otherwise?
As I understand the problematic entity-is-not-YET existing case can mostly incur with default content imports. If so, it sounds like something we could address there by making sure we invalidate caches of all imported entities after default content import. Or can we think of other cases also that could cause that friction?