Exceptions on setting entity reference field with integer target ID and entity object [#2851149]

Problem/Motivation

When setting an entity reference field with target ID as integer and next to it provide the entity object an exception will be thrown.

Proposed resolution

The reason is that in EntityReferenceItem::setValue we do a type comparison:

$entity_id = $this->get('entity')->getTargetIdentifier();
...
$entity_id !== $values['target_id']

However $entity->id() returns the ID as a string, so the comparison will fail.

We should compare with "!=" instead of with "!==".

Remaining tasks

Review & Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

Comment	File	Size	Author
#20	2849861-d6-textarea-format-2-reroll.patch	2.89 KB	joelpittet
#20	8.3.x: PHP 5.6 & MySQL 5.5 21,427 pass
#9	interdiff-3-9.txt	1.13 KB	hchonov
#9	2851149-9.patch	2.48 KB	hchonov
#9	8.3.x: PHP 5.6 & MySQL 5.5 21,427 pass PHP 5.6 & MySQL 5.5 21,633 pass 8.4.x: PHP 5.6 & MySQL 5.5 21,659 pass
#3	2851149-3.patch	2.15 KB	hchonov
#3	8.4.x: PHP 5.6 & MySQL 5.5 21,605 pass
#2	2851149-2-PASS.patch	2.03 KB	hchonov
#2	8.4.x: PHP 5.6 & MySQL 5.5 21,606 pass
#2	2851149-2-FAIL.patch	1.03 KB	hchonov
#2	8.4.x: PHP 5.6 & MySQL 5.5 21,598 pass, 2 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

9 February 2017 at 13:54

hchonov created an issue. See original summary.

Comment #2

hchonov

Bulgarian

🇪🇺🇩🇪🇧🇬

CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 9 February 2017 at 13:55

File	Size
2851149-2-FAIL.patch	1.03 KB
8.4.x: PHP 5.6 & MySQL 5.5 21,598 pass, 2 fail
2851149-2-PASS.patch	2.03 KB
8.4.x: PHP 5.6 & MySQL 5.5 21,606 pass

Comment #3

hchonov

Bulgarian

🇪🇺🇩🇪🇧🇬

CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 9 February 2017 at 14:16

File	Size
2851149-3.patch	2.15 KB
8.4.x: PHP 5.6 & MySQL 5.5 21,605 pass

I've been asked in IRC by @amateescu to put the code snippet to an existing test method, this is the only change to the PASS patch.

Comment #4

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 9 February 2017 at 14:18

Version:	8.4.x-dev	» 8.3.x-dev
Status:	Needs review	» Reviewed & tested by the community

Looks great :)

Comment #5

9 February 2017 at 14:50

The last submitted patch, 2: 2851149-2-FAIL.patch, failed testing.

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 9 February 2017 at 15:34

However $entity->id() returns the ID as a string, so the comparison will fail.

/facepalm

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -190,7 +190,7 @@ public function setValue($values, $notify = TRUE) {
-        if (!$this->entity->isNew() && $values['target_id'] !== NULL && ($entity_id !== $values['target_id'])) {
+        if (!$this->entity->isNew() && $values['target_id'] !== NULL && ($entity_id != $values['target_id'])) {

I think it'd be good to add an explicit comment here about the use of a non-strict comparison.

Comment #7

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 9 February 2017 at 16:18

Status:

Reviewed & tested by the community

» Needs work

Yes let's add the comment. Or alternatively should we cast the result of $entity->id() somewhere beforehand?

Comment #8

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 9 February 2017 at 16:53

Or alternatively should we cast the result of $entity->id() somewhere beforehand?

That's a bit dangerous because entity IDs can be integers or strings, and I don't think looking at the base field definitions just for this cast is justifiable.

Comment #9

hchonov

Bulgarian

🇪🇺🇩🇪🇧🇬

CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 9 February 2017 at 17:28

Status:

Needs work

» Needs review

File	Size
2851149-9.patch	2.48 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,427 pass PHP 5.6 & MySQL 5.5 21,633 pass 8.4.x: PHP 5.6 & MySQL 5.5 21,659 pass
interdiff-3-9.txt	1.13 KB

File

Size

2851149-9.patch

2.48 KB

8.4.x:
PHP 5.6 & MySQL 5.5 21,659 pass

interdiff-3-9.txt

1.13 KB

Adding a comment why we have to do a non-strict comparison.

Comment #10

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented 9 February 2017 at 17:37

Status:

Needs review

» Reviewed & tested by the community

Nice :)

Comment #11

20 February 2017 at 04:28

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 9: 2851149-9.patch, failed testing.

Comment #12

hchonov

Bulgarian

🇪🇺🇩🇪🇧🇬

CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 20 February 2017 at 09:26

Status:

Needs work

» Reviewed & tested by the community

It has been a CI error, therefore putting back to RTBC.

Comment #13

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 20 February 2017 at 14:05

Status:	Reviewed & tested by the community	» Needs work
Issue tags:		+Needs re-roll

This looks good to me, the @todos point to an issue that's close to ready but isn't going to make it into 8.3.x, whereas this still potentially could. Doesn't apply to 8.4.x though.

Comment #14

hchonov

Bulgarian

🇪🇺🇩🇪🇧🇬

CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 20 February 2017 at 19:03

@catch, I've just tried to apply the patch and it applies on both 8.3.x and 8.4.x. I've put it for retesting once more for both 8.3.x and 8.4.x just in case.

Comment #15

hchonov

Bulgarian

🇪🇺🇩🇪🇧🇬

CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented 20 February 2017 at 19:33

Status:	Needs work	» Reviewed & tested by the community
Issue tags:	-Needs re-roll

It looks like that the patch is applying as the tests are currently running, therefore I am removing the "Needs re-roll" tag and putting it back to RTBC as nothing has been changed on the patch.

Comment #16

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 6 March 2017 at 13:42

Status:

Reviewed & tested by the community

» Patch (to be ported)

Committed 4bdfd06 and pushed to 8.4.x. Thanks!

Comment #17

6 March 2017 at 13:42

alexpott committed 4bdfd06 on 8.4.x

Issue #2851149 by hchonov, amateescu, Wim Leers: Exceptions on setting...

Comment #18

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Chapter Three commented 6 March 2017 at 16:49

Status:

Patch (to be ported)

» Fixed

@catch, @cilefen, @xjm and I just decided that patch rules apply to 8.3.x up until RC2 so we can back port this.

Committed 041b208 and pushed to 8.3.x. Thanks!

Comment #19

6 March 2017 at 16:49

alexpott committed 041b208 on 8.3.x

Issue #2851149 by hchonov, amateescu, Wim Leers: Exceptions on setting...

Comment #20

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 6 March 2017 at 19:30

Status:

Fixed

» Reviewed & tested by the community

File	Size
2849861-d6-textarea-format-2-reroll.patch	2.89 KB
8.3.x: PHP 5.6 & MySQL 5.5 21,427 pass

Comment #21

joelpittet

English

Vancouver

CreditAttribution: joelpittet at The University of British Columbia commented 6 March 2017 at 19:31

Status:

Reviewed & tested by the community

» Fixed

Whoops wrong issue.

Comment #22

20 March 2017 at 19:34

Status:

Fixed

» Closed (fixed)

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

Exceptions on setting entity reference field with integer target ID and entity object

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Thank you to these Drupal contributors

News items

Our community

Documentation

Drupal code base

Governance of community