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

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because 0 is valid ID for anon user
Issue priority Normal
Disruption None
CommentFileSizeAuthor
#8 empty-entity-ids-227935-5.patch2.57 KBAnushka-mp
#4 empty-entity-ids-2327935-4.patch2.55 KBarla
#4 empty-entity-ids-2327935-4-TESTS-ONLY.patch1 KBarla
#3 empty-entity-ids-2327935-3.patch1.55 KBarla

Comments

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs tests

Agreed with the issue itself. Let's just ensure that we have test coverage for that in the future.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

For ConfigEntities, '' and NULL are not valid IDs. 0 and '0' are valid.
I can't speak for ContentEntities

arla’s picture

arla commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new empty-entity-ids-2327935-3.patch1.55 KB

Yeah, 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.

arla’s picture

arla commented
Related issues: +#1915056: Use entity reference for taxonomy parents
StatusFileSize
new empty-entity-ids-2327935-4-TESTS-ONLY.patch1 KB
new empty-entity-ids-2327935-4.patch2.55 KB

This 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.

The last submitted patch, 4: empty-entity-ids-2327935-4-TESTS-ONLY.patch, failed testing.

cbr queued 4: empty-entity-ids-2327935-4.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: empty-entity-ids-2327935-4.patch, failed testing.

Anushka-mp’s picture

Anushka-mp commented
StatusFileSize
new empty-entity-ids-227935-5.patch2.57 KB
Anushka-mp’s picture

Anushka-mp commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks good to me, completed the beta eval template too

brahmjeet789 queued 8: empty-entity-ids-227935-5.patch for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice 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!

  • alexpott committed adb97da on 8.0.x 
    Issue #2327935 by Arla, Anushka-mp: Allow empty entity IDs in...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, we do: https://github.com/larowlan/default_content/issues/33

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.