Problem/Motivation

Currently to access to the entity or language related to a field item is necessary the following code:
$entity = $this->getParent()->getParent();
$language = $this->getParent()->getParent()->language();

Proposed resolution

Code would be cleaner if we added methods getEntity() and language() in FieldInterface / FieldItemInterface:
$entity = $this->getEntity();
$language = $this->language();

API changes

Only additions - Two methods added to FieldInterface / FieldItemInterface

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Title: Add helper methods getEntiy() and language() in FieldItemInterface » Add helper methods getEntity() and language() in FieldItemInterface
Status: Active » Needs review
FileSize
4.05 KB

Attaching patch that adds these new methods.
Regards

yched’s picture

Way cool !

Actually, we would need those methods on Core/Entity/Field/Field too (they just need to go one level up)
Then the implementations in FieldItemBase would do $this->getParent()->getEntity() / $this->getParent()->language()

+++ b/core/modules/node/lib/Drupal/node/Plugin/Validation/Constraint/NodeChangedConstraintValidator.phpundefined
@@ -22,7 +22,7 @@ public function validate($value, Constraint $constraint) {
       // We are on the field item level, so we need to go two levels up for the
       // node object.
-      $node = $this->context->getMetadata()->getTypedData()->getParent()->getParent();
+      $node = $this->context->getMetadata()->getTypedData()->getEntity();

The comment should be removed then.

Status: Needs review » Needs work
Issue tags: -API change, -Field API

The last submitted patch, helper_methods-2061331-1.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Field API

#1: helper_methods-2061331-1.patch queued for re-testing.

plopesc’s picture

Hello
In this case code in Field.php should be:

/**
 * {@inheritdoc}
 */
public function getEntity() {
  return $this->getParent();
}

/**
 * {@inheritdoc}
 */
public function language() {
  return $this->getEntity()->language();
}

and the getEntity() method is duplicating the getParent() functionality. Is this right?
Regards

yched’s picture

Title: Add helper methods getEntity() and language() in FieldInterface / FieldItemInterface » Add helper methods getEntity() and language() in FieldItemInterface

getEntity() method is duplicating the getParent()

Yes. getParent() is here because of TypedDataInterface, which might go away at some point. getEntity() would be part of FieldInterface.

yched’s picture

Title: Add helper methods getEntity() and language() in FieldItemInterface » Add helper methods getEntity() and language() in FieldInterface / FieldItemInterface

retitling

plopesc’s picture

Title: Add helper methods getEntity() and language() in FieldItemInterface » Add helper methods getEntity() and language() in FieldInterface / FieldItemInterface
FileSize
4.5 KB
7.13 KB

Hello
Here it is the new round.
Including getEntity() and language() in FieldInterface.
Regards.

yched’s picture

Issue tags: -Entity Field API

Sweet.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldInterface.phpundefined
@@ -36,6 +36,22 @@
+   * @return Drupal\Core\TypedData\ComplexDataInterface

missing the leading '\' (same in FieldItemInterface)

[edit: + we should typehint those with EntityInterface]

yched’s picture

Issue tags: +Entity Field API

This would need the approval of the Entity API folks.

plopesc’s picture

Fixing docblock

plopesc’s picture

Issue tags: +Entity Field API

Adding removed tag.
Damn browser cache :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, and will be very helpful in various places.
Thanks !

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

git ac https://drupal.org/files/helper_methods-2061331-11_0.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7282  100  7282    0     0   2318      0  0:00:03  0:00:03 --:--:--  2748
error: patch failed: core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php:77
error: core/modules/text/lib/Drupal/text/Plugin/field/field_type/TextItemBase.php: patch does not apply
yched’s picture

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

Reroll

yched’s picture

Issue tags: -Needs reroll
tstoeckler’s picture

Still looks great.

yched’s picture

bump - would really be nice to have this for #2075095: Widget / Formatter methods signatures needlessly complex

yched’s picture

#15: helper_methods-2061331-15.patch queued for re-testing.

plach’s picture

It seems to me we are not taking into account field translatability: if we are dealing with an untranslatable field and we have an entity object not referring to the original language, the correct language to be used is Language::LANGCODE_DEFAULT ('und').

yched’s picture

@plach: Well, as the replacements in the patch shows, there are currently already places in core that consider that the language of a Field/FieldItem is the language of the parent entity.

Whether that assumption is right or wrong (and all the more if it is wrong), this shows that we need a Field/FieldItem::language() method.

Short of that, this requires any code that works with a Field/FieldItem to also receive its language in a separate param - "here is the langcode that the calling code considers to be the language of this FieldItem according to some complex logic". This makes APIs much heavier and brittle - the call chain betwwen field_attach_form() & actual methods in widgets is a perfect example.

Field values are stored with an assigned langcode, that langode should be a simple API step away when working with the Field object built out of those values.

plach’s picture

Whether that assumption is right or wrong (and all the more if it is wrong), this shows that we need a Field/FieldItem::language() method.

Sure, I am just saying that if we introduce a method to encapsulate this logic it should take into account field translatability, otherwise we would be officializing a wrong behavior. I'd be tempted to mark this NW.

yched’s picture

Status: Reviewed & tested by the community » Needs work

So yes, NW it is :-/
I'll try to look how to have Field objects carry their language.

plach’s picture

I'll try to look how to have Field objects carry their language.

Can't we just check whether field is translatable on instantiation and in that case make ::language() return Language::LANGCODE_DEFAULT?

yched’s picture

and what if the field is translatable, and some fallback logic has been applied ? then it becomes more complex, and tracking langcodes becomes messy - which is why current code has to pass along the langcode for each $field_item in separate params.

It's sick: field values have *one* langcode in storage, and they are turned into objects at runtime, so those objects should just carry their own language, not expect the external code to somehow know their own language.

yched’s picture

@plach: opened a separate issue and posted a patch in #2078625: Field/FieldItem value objects should carry their language

yched’s picture

I bumped #2078625: Field/FieldItem value objects should carry their language to critical - FieldItem classes cannot do their job properly without having a langcode, core is broken on that regard atm.
Patch over there is ready from my POV.

yched’s picture

Title: Add helper methods getEntity() and language() in FieldInterface / FieldItemInterface » Add helper methods getEntity() in FieldInterface / FieldItemInterface

Methods to get the language were added in #2078625: Field/FieldItem value objects should carry their language, which leaves us with the Field/FieldItem::getEntity() helpers here.

It's only recently :-/ that I figured out that TypedDataInterface had getRoot(), which happens to do what we'd expect from a getEntity() method: Field & FieldItem both inherit the implementation from TypedData, that does what we want.

So, strictly speaking we don't *need* new getEntity() helper methods. It's just that getRoot() comes from a broader API, and the method name makes little sense to the actual business logic at hand when working with Field / FieldItem objects - same old drawback about an ubiquitous TypedDataInterface...

Thoughts ?

Berdir’s picture

I think it makes sense to add this method for two reasons:

a) getRoot() might go away, at least from EntityInterface
b) Imagine this: $node->reference->entity->some_field->getRoot(). What's that going to return? It will be the referenced entity at the moment because Entity doesn't implement getRoot(), but it's pretty ambiguous. Whereas getEntity() there makes sense I think and gives you the entity this field/item belongs to.

yched’s picture

$node->reference->entity->some_field->getRoot()

Yes, I checked before posting #28, it does "stop" at the referenced entity and doesn't go higher - which is what we want.
But yes, I had to check to convince myself :-) So, fully agreed.

yched’s picture

Status: Needs work » Needs review
FileSize
19.26 KB
18.79 KB

Updated patch:
- does not add Field/FieldItem::language() anymore, we have getLangcode() for that
- moves getEntity() around a bit in classes and interfaces
- fixes a couple more places where we want to call getEntity() & getLangcode()

Status: Needs review » Needs work

The last submitted patch, field_helper_methods-2061331-31.patch, failed testing.

yched’s picture

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityReferenceItemNormalizer.php
@@ -46,8 +46,8 @@ public function normalize($field_item, $format = NULL, array $context = array())
-    $field_name = $field_item->getParent()->getName();
+    $field_name = $field_item->getFieldDefinition()->getFieldName();

Slap on the wrist for changing code that's not strictly related to the task at hand.
getFieldDefinition() still doesn't work for base fields. Reverted.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
@@ -391,12 +391,14 @@ protected function checkIntrospection($entity_type) {
+    $this->assertIdentical($field->getEntity(), $entity);

Protip: when comparing entity objects, don't let $this->assert*() generate an assert message or you'll get into infinite recursion...

yched’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
18.71 KB

Ignore patches in #33, tonight is not my night...
Interdiff is with #31

yched’s picture

Priority: Normal » Major
Issue tags: -API change +API addition

API addition rather than API change.
Also, at least major, since the patch fixes the places that need the field langcode rather than the entity langcode.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Sweet, this makes field life a lot saner.

andypost’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentNewValue.php
    @@ -26,8 +26,7 @@ public function getValue() {
    +      $entity = $this->parent->getEntity();
    

    is this parent still needed?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
    @@ -391,12 +391,14 @@ protected function checkIntrospection($entity_type) {
         $this->assertIdentical($field->getRoot(), $entity, 'Entity is root object.');
    +    $this->assertIdentical($field->getEntity(), $entity, 'getEntity() returns the entity.');
    ...
         $this->assertIdentical($field_item->getRoot(), $entity, 'Entity is root object.');
    +    $this->assertIdentical($field_item->getEntity(), $entity, 'getEntity() returns the entity.');
    

    Awesome! both now have tests!

yched’s picture

@andypost:

+++ b/core/modules/comment/lib/Drupal/comment/CommentNewValue.php
@@ -26,8 +26,7 @@ public function getValue() {
+      $entity = $this->parent->getEntity();

is this parent still needed?

Yes, because this code lives in a property class, and this patch doesn't add a getEntity() method on those (there's no common base class where we could put an implementation).
So, $this->parent to get the FieldItem object, and there you can call getEntity().

yched’s picture

Note that #2075095: Widget / Formatter methods signatures needlessly complex is more or less blocked on this - patch over there contains bits of this patch here, so getting it in soonish would help :-p

yched’s picture

Reroll after #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property, that took care of the getLangcode() fixes in text fields.

yched’s picture

Oops, #41 introduced whitespaces.

yched’s picture

alexpott’s picture

Title: Add helper methods getEntity() in FieldInterface / FieldItemInterface » Change notice: Add helper methods getEntity() in FieldInterface / FieldItemInterface
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Let's update https://drupal.org/node/2064123

Committed 678df35 and pushed to 8.x. Thanks!

yched’s picture

Title: Change notice: Add helper methods getEntity() in FieldInterface / FieldItemInterface » Add helper methods getEntity() in FieldInterface / FieldItemInterface
Status: Active » Fixed
Issue tags: -Needs change record

Thanks Alex :-)

Updated https://drupal.org/node/2064123, but I'm wondering whether it's the best/only place to mention getEntity() / getLangcode(). Those should be mentioned in the change notice for (what was then called) EntityNG, which introduced field values as classed objects - but I can't seem to find it :-/

yched’s picture

Title: Add helper methods getEntity() in FieldInterface / FieldItemInterface » Add getEntity() helper in FieldInterface / FieldItemInterface

fix grammar for posterity...

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

Anonymous’s picture

Issue summary: View changes

update for FieldInterafce