As per #2015693-11: Convert field type to typed data plugin for link module :
"The goal is to make our FieldItem classes be usable for configurable fields (that can be added through the UI) as well as base entity fields (that are hardcoded in the Entity definition).
This means relying as little as possible on the Field / FieldInstance definition structures that are specific to configurable fields, and instead use the FieldDefinitionInterface."
I asked for the corresponding changes in #2015693: Convert field type to typed data plugin for link module and #2015691: Convert field type to typed data plugin for number module, this issue is about fixing field types that already have been converted in core.
Comment | File | Size | Author |
---|---|---|---|
#10 | field_types-FieldDefinitionInterface-2051177-10.patch | 9.18 KB | yched |
#5 | field_types-FieldDefinitionInterface-2051177-5.patch | 7.07 KB | yched |
#5 | interdiff.txt | 4.47 KB | yched |
#4 | field_types-FieldDefinitionInterface-2051177-3.patch | 4.8 KB | yched |
#4 | interdiff.txt | 612 bytes | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch.
Even in the settingsForm() & instanceSettingsForm() methods, that can only be called on configurable fields, it is easier & more consistent to fetch settings through FieldDefinitionInterface::getFieldSetting().
One thing that bugs me, and that we left untouched in #2015691: Convert field type to typed data plugin for number module and in the attached patch, is the schema() method. It is only called for configurable fields, and specifically receives a configurable Field config entity structure.
In this method, you're only supposed to use field-level settings, not instance-level settings (by definition, only field-level settings should affect the schema, not instance-level settings).
$field->setting['some_field_setting'] works.
$field->setting['some_instance_setting'] will raise a notice, which is good here, you shouldn't do that.
$field->getFieldSetting('some_instance_setting') will work (returns the default value for the instance setting), which silently lets you write broken code.
So either we leave the code in schema() use a different syntax/convention than the rest of the class ($field->setting) - which seems confusing,
or we unify on a syntax that might lead to nasty/subtle bugs down the road...
Comment #2
yched CreditAttribution: yched commentedEr, patch.
Comment #4
yched CreditAttribution: yched commentedWoops. settingsForm() / hasData() is being taken care of in #2051157: pass $has_data as a param to ConfigFieldItem::settingsForm(), but for now this still needs to be done using getInstance()->getField()->hasData().
Comment #5
yched CreditAttribution: yched commentedThe various conversions being currently worked on show that it would be very handy to have shortcuts for:
$this->getFieldDefinition()->getSetting('foo')
$this->getFieldDefinition()->getSettings()
WidgetBase and FormatterBase already have exactly the same protected helpers, so it only makes sense to add similar ones to FieldItemBase.
Comment #6
amateescu CreditAttribution: amateescu commentedThe patch looks good to me. Do we want to further discuss the handling of schema() in here or go with rapid improvements and move that discussion to a new issue?
Comment #7
yched CreditAttribution: yched commentedNew issue IMO. Let's get moving :-)
Comment #8
amateescu CreditAttribution: amateescu commentedSure thing.
Comment #9
alexpottLooks like we can also update
Drupal\number\Plugin\field\field_type\NumberItemBase
andDrupal\number\Plugin\field\field_type\DecimalItem
to use the newgetFieldSettings()
methodPlus
Drupal\number\Plugin\field\field_type\DecimalItem::preSave
can use the newgetFieldSetting()
methodComment #10
yched CreditAttribution: yched commentedYes, I was planning to do number in a followup while this one was RTBC, but sure, we can do it now as well. There's no other field type conversion patch ready enough to be waiting on the helpers, so it makes no real difference.
Comment #11
amateescu CreditAttribution: amateescu commentedOne more try :)
Edit: Also, some field type conversions are waiting for this to go in so they can be re-rolled to use these helpers, like #2017139: Convert field type to typed data plugin for datetime module .
Comment #12
alexpottCommitted 8bed469 and pushed to 8.x. Thanks!
Created to clean up LinkItem #2056991: Use getFieldSetting() instead of getDefinition()->getFieldSetting() in LinkItem
Comment #13
claudiu.cristeaThis may be related #2060003: Wrong precedence in Field::getFieldSetting[s]() when setting appears in both field and instance?
Comment #14.0
(not verified) CreditAttribution: commentededit