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 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
Comment | File | Size | Author |
---|---|---|---|
#42 | field_helper_methods-2061331-42.patch | 16.4 KB | yched |
#41 | field_helper_methods-2061331-41.patch | 16.37 KB | yched |
#41 | interdiff.txt | 528 bytes | yched |
#40 | field_helper_methods-2061331-40.patch | 17.11 KB | yched |
#33 | field_helper_methods-2061331-33.patch | 21.47 KB | yched |
Comments
Comment #1
plopescAttaching patch that adds these new methods.
Regards
Comment #2
yched CreditAttribution: yched commentedWay 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()
The comment should be removed then.
Comment #4
plopesc#1: helper_methods-2061331-1.patch queued for re-testing.
Comment #5
plopescHello
In this case code in Field.php should be:
and the getEntity() method is duplicating the getParent() functionality. Is this right?
Regards
Comment #6
yched CreditAttribution: yched commentedYes. getParent() is here because of TypedDataInterface, which might go away at some point. getEntity() would be part of FieldInterface.
Comment #7
yched CreditAttribution: yched commentedretitling
Comment #8
plopescHello
Here it is the new round.
Including getEntity() and language() in FieldInterface.
Regards.
Comment #9
yched CreditAttribution: yched commentedSweet.
missing the leading '\' (same in FieldItemInterface)
[edit: + we should typehint those with EntityInterface]
Comment #10
yched CreditAttribution: yched commentedThis would need the approval of the Entity API folks.
Comment #11
plopescFixing docblock
Comment #12
plopescAdding removed tag.
Damn browser cache :)
Comment #13
yched CreditAttribution: yched commentedLooks good to me, and will be very helpful in various places.
Thanks !
Comment #14
alexpottNeeds a reroll
Comment #15
yched CreditAttribution: yched commentedReroll
Comment #16
yched CreditAttribution: yched commentedComment #17
tstoecklerStill looks great.
Comment #18
yched CreditAttribution: yched commentedbump - would really be nice to have this for #2075095: Widget / Formatter methods signatures needlessly complex
Comment #19
yched CreditAttribution: yched commented#15: helper_methods-2061331-15.patch queued for re-testing.
Comment #20
plachIt 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'
).Comment #21
yched CreditAttribution: yched commented@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.
Comment #22
plachSure, 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.
Comment #23
yched CreditAttribution: yched commentedSo yes, NW it is :-/
I'll try to look how to have Field objects carry their language.
Comment #24
plachCan't we just check whether field is translatable on instantiation and in that case make
::language()
returnLanguage::LANGCODE_DEFAULT
?Comment #25
yched CreditAttribution: yched commentedand 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.
Comment #26
yched CreditAttribution: yched commented@plach: opened a separate issue and posted a patch in #2078625: Field/FieldItem value objects should carry their language
Comment #27
yched CreditAttribution: yched commentedI 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.
Comment #28
yched CreditAttribution: yched commentedMethods 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 ?
Comment #29
BerdirI 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.
Comment #30
yched CreditAttribution: yched commented$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.
Comment #31
yched CreditAttribution: yched commentedUpdated 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()
Comment #33
yched CreditAttribution: yched commentedSlap 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.
Protip: when comparing entity objects, don't let $this->assert*() generate an assert message or you'll get into infinite recursion...
Comment #34
yched CreditAttribution: yched commentedIgnore patches in #33, tonight is not my night...
Interdiff is with #31
Comment #35
yched CreditAttribution: yched commentedAPI 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.
Comment #36
swentel CreditAttribution: swentel commentedSweet, this makes field life a lot saner.
Comment #37
andypostis this parent still needed?
Awesome! both now have tests!
Comment #38
yched CreditAttribution: yched commented@andypost:
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().
Comment #39
yched CreditAttribution: yched commentedNote 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
Comment #40
yched CreditAttribution: yched commentedReroll after #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property, that took care of the getLangcode() fixes in text fields.
Comment #41
yched CreditAttribution: yched commentedOops, #41 introduced whitespaces.
Comment #42
yched CreditAttribution: yched commentedReroll after #2082509: Generalize node changed constraint to entity changed constraint
Comment #43
alexpottLet's update https://drupal.org/node/2064123
Committed 678df35 and pushed to 8.x. Thanks!
Comment #44
yched CreditAttribution: yched commentedThanks 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 :-/
Comment #45
yched CreditAttribution: yched commentedfix grammar for posterity...
Comment #46.0
(not verified) CreditAttribution: commentedupdate for FieldInterafce