Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Currently ChainEntityResolver returns NULL if TargetIdResolver returns an empty value. Although NULL is not, empty values like "", "0" and 0 are valid entity ID's. The resolver should not return NULL for those.
An example where this matters is on entity_reference fields where target_id is 0.
Proposed resolution
Replace the if($id = resolve()) {} check with $id = resolve; if (isset($id)) {} in ChainEntityResolver and in EntityReferenceItemNormalizer.
Remaining tasks
None
User interface changes
None
API changes
None
Beta phase evaluation
| Issue category | Bug because 0 is valid ID for anon user |
|---|---|
| Issue priority | Normal |
| Disruption | None |
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | empty-entity-ids-227935-5.patch | 2.57 KB | Anushka-mp |
| #4 | empty-entity-ids-2327935-4.patch | 2.55 KB | Arla |
| #4 | empty-entity-ids-2327935-4-TESTS-ONLY.patch | 1 KB | Arla |
| #3 | empty-entity-ids-2327935-3.patch | 1.55 KB | Arla |
Comments
Comment #1
dawehnerAgreed with the issue itself. Let's just ensure that we have test coverage for that in the future.
Comment #2
tim.plunkettFor ConfigEntities, '' and NULL are not valid IDs. 0 and '0' are valid.
I can't speak for ContentEntities
Comment #3
ArlaYeah, some tests would be needed to prove the issue. Until then, here's the suggested fix in code. I guess it will pass, because afaik no tests care about this yet.
Comment #4
ArlaThis should motivate the fix in ChainEntityResolver. It's my first go with PHPUnit :)
Btw, @Berdir suggested that TargetIdResolver should actually check if the referenced entity exists. Not sure if this issue is the proper place to do that, maybe if we broaden the scope/title a bit.
Also adding a related issue where we want to be able to use, and serialize, 0 for target_id.
Comment #8
Anushka-mp CreditAttribution: Anushka-mp commentedComment #9
Anushka-mp CreditAttribution: Anushka-mp commentedComment #10
larowlanLooks good to me, completed the beta eval template too
Comment #12
alexpottNice find. I wondering if we should expand coverage of testing entity ids of 0 or '0' since this might not be the only place we have problems.
This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed adb97da and pushed to 8.0.x. Thanks!
Comment #14
BerdirYeah, we do: https://github.com/larowlan/default_content/issues/33