Updated: Comment #0

Problem/Motivation

EntityReferenceItem redirects magic ->value calls to ->target_id, which is obscure as there is no value property on entity reference items.

Proposed resolution

Remove that magic redirect.

Remaining tasks

Write a patch that either converts $reference->value calls either to proper domain methods of the particular entity object or to $reference->target_id calls for configurable fields.

User interface changes

none.

API changes

Entity reference items cannot be addressed with ->value anymore.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
2.97 KB

Let's see what happens then :)

Status: Needs review » Needs work

The last submitted patch, 1: entity-reference-item-value-2188075-1.patch, failed testing.

Berdir’s picture

This should work better, properly checking for the existence of a field.

Status: Needs review » Needs work

The last submitted patch, 3: entity-reference-item-value-2188075-3.patch, failed testing.

Berdir’s picture

Ok, comment explodes, because entity_id is (an incorrectly hardcoded to node) entity reference field but used with ->value all over the place.

Let's wait for #2028025: Expand CommentInterface to provide methods

Berdir’s picture

The last submitted patch, 3: entity-reference-item-value-2188075-3.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.06 KB

Re-roll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: entity-reference-item-value-2188075-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

API's changed :)

And I didn't mean to set to RTBC, obviously.

Berdir’s picture

And now forgot the patch. #notmyday :p

Status: Needs review » Needs work

The last submitted patch, 11: entity-reference-item-value-2188075-10.patch, failed testing.

Berdir’s picture

yched’s picture

Oh, yes yes yes please

amateescu’s picture

amateescu’s picture

Status: Postponed » Needs review
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great.

The test fixes show the impact of this change and I think this helps to make clear that ->value is *not* a standard/universal thing that always exists, it's just a common pattern among many single-value field types or field types with that + some additional support properties.

While it might confuse developers in the first moment, I think it will help to understand this, the fact that ->value so far magically worked despite not being defined anywhere so far did not.

Setting to RTBC, I worked on the initial patch but the only things left that I "wrote" are the removed overridden get()/set methods, so I think that's OK.

It's kind of an API change, even though it never really was an API, just a workaround that was added when the main property concept didn't exist yet.

yched’s picture

Right. +1 for the exact reasons @berdir stated in #17.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: entity-reference-item-value-2188075-15.patch, failed testing.

Berdir’s picture

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Was a testbot problem.

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit aef08a5 on 8.x by catch:
    Issue #2188075 by Berdir, amateescu, xjm: Remove magic getter of...

Status: Fixed » Closed (fixed)

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