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.
Easier DX : get the $field from the $instance object.
Thus, rename the FieldInstance::$field_name property.
Comment | File | Size | Author |
---|---|---|---|
#21 | field-instance-getField-1967106-21.patch | 13.01 KB | swentel |
#21 | interdiff.txt | 748 bytes | swentel |
#15 | field-instance-getField-1967106-15.patch | 13.01 KB | yched |
#15 | interdiff.txt | 11.73 KB | yched |
#10 | get-field-1967106.10.interdiff.txt | 455 bytes | larowlan |
Comments
Comment #1
larowlanTagging
Comment #2
swentel CreditAttribution: swentel commentedtagging
Comment #3
xjmPostponing on the main issue.
Comment #4
yched CreditAttribution: yched commentedThis should probably be one of the 1st followup we tackle, since it will have major impact on the shape of the code rewrites that happen in #1953408: Remove ArrayAccess BC layer from field config entities.
Comment #5
yched CreditAttribution: yched commentedUn-postponing.
Comment #6
yched CreditAttribution: yched commentedComment #7
larowlanSounds straight forward
Comment #8
larowlanlike so?
Comment #10
larowlanMissed the namespace
Comment #11
xjmWhy are we adding an interface? Is this the only method on FieldInstance that doesn't belong to ConfigEntityBase or ArrayAccess? Interesting.
Seems to be missing the actual declaration? :)
Comment #12
yched CreditAttribution: yched commentedYay, thanks @larowlan for jumping on this :-)
Yeah, I'm not sure about the interface either (and yes, there are other methods in FieldInstance currently that would need to go to the interface if we introduced one).
Sure, we will definitely be hinting on FieldInstance in function / method params (we already currently are at places, and the number will grow in #1953408: Remove ArrayAccess BC layer from field config entities), and core standards state that we should favor hinting on interfaces.
OTOH, this is not really related to this very task, and
- no other existing ConfigEntity in core right now provide their own interface, aside from implementing ConfigEntityInterface itself.
- the situation is a bit blurry for content entities, and is being discussed in #1391694: Use type-hinting for entity-parameters.
So I'd really suggest leaving this question of interfaces aside for now.
Comment #13
yched CreditAttribution: yched commented// @todo Revisit that in favor of a getField() method.
So, If we consider that we are in the task that is supposed to get rid of that @todo ;-), we should:
Good, but then also unset($values['field_name'])
No more assigning $values['field_name'], just raise the exception if (empty($this->field))
add a case to return $this->field->id when $offset is 'field_name', to keep the BC $instance['field_name'] syntax working.
It looks like the code in __construct() has wiped any chance of this actually happening ?
Comment #14
yched CreditAttribution: yched commentedThis is going to be useful in several places. Taking a crack at this.
Comment #15
yched CreditAttribution: yched commentedPatch does as described in #13.
Additionally:
- Takes care of FieldInstance serialization/unserialization : we don't want to serialize $this->field within the $instance.
Also needed because $instance also persists the widget Plugin object ($this->widgetPlugin), and we don't want that one serialized either (FieldInstance had a __sleep() method that took care of that, but it got lost in #1735118: Convert Field API to CMI)
- While I'm at it, does the same for Field, since that stores derived data that we don't want serialized either (field schema, storage details).
Comment #16
larowlanSo the interface is gone? I think that as a general rule any methods on an entity not from {Content|Config}EntityInterface should warrant a new interface.
Comment #17
yched CreditAttribution: yched commented@larowlan : see #11 & #12.
Possibly, but FieldInstance already has methods that are not part of ConfigEntityInterface, the patch doesn't change that fact. So let's not tie it to this discussion when it doesn't need to :-)
Comment #18
larowlanFair point, interface is scope creep for this issue
Comment #19
tim.plunkettThat will be true after #1391694: Use type-hinting for entity-parameters, I updated that to have interfaces for both Field and FieldInstance
Comment #20
yched CreditAttribution: yched commentedUnassigning. Needs reviews now :-)
Comment #21
swentel CreditAttribution: swentel commentedSolid cleanup, me like. Nitpick patch attached (redundant space). Assuming it's still green :)
Comment #22
webchickThis looks like a nice DX improvement, but in no way is it major. :)
Committed and pushed to 8.x. Thanks!
Comment #23
yched CreditAttribution: yched commentedFYI : #1977206: Default serialization of ConfigEntities - proposal to move the serialize() / unserialize() methods from Field / FieldInstance to ConfigEntityBase.
@webchick: yeah, sorry about the "major" thing, I guess got a bit carried away.
Was mostly a way to help us sort out priorities between the numerous followup issues opened after the Field / CMI patch, but right, bad idea...