Make existing ConfigFieldItem subclasses usable by base fields [#2051177]

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
#10
#5	field_types-FieldDefinitionInterface-2051177-5.patch	7.07 KB	yched
#5
#5	interdiff.txt	4.47 KB	yched
#4	field_types-FieldDefinitionInterface-2051177-3.patch	4.8 KB	yched
#4
#4	interdiff.txt	612 bytes	yched
#2	field_types-FieldDefinitionInterface-2051177-1.patch	4.8 KB	yched
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 26 July 2013 at 13:28

Status:

Needs review

» Active

Patch.

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 commented 26 July 2013 at 13:22

Status:

Active

» Needs review

File	Size
field_types-FieldDefinitionInterface-2051177-1.patch	4.8 KB

Er, patch.

Comment #3

26 July 2013 at 14:47

Status:

Active

» Needs work

The last submitted patch, field_types-FieldDefinitionInterface-2051177-1.patch, failed testing.

Comment #4

yched CreditAttribution: yched commented 26 July 2013 at 15:40

Status:

Needs work

» Needs review

File	Size
interdiff.txt	612 bytes
field_types-FieldDefinitionInterface-2051177-3.patch	4.8 KB

Woops. 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 commented 26 July 2013 at 21:26

File	Size
interdiff.txt	4.47 KB
field_types-FieldDefinitionInterface-2051177-5.patch	7.07 KB

The 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 commented 27 July 2013 at 22:57

The 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 commented 27 July 2013 at 23:48

New issue IMO. Let's get moving :-)

Comment #8

amateescu CreditAttribution: amateescu commented 27 July 2013 at 23:48

Status:

Needs review

» Reviewed & tested by the community

Sure thing.

Comment #9

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 30 July 2013 at 13:06

Status:

Reviewed & tested by the community

» Needs work

Looks like we can also update Drupal\number\Plugin\field\field_type\NumberItemBase and Drupal\number\Plugin\field\field_type\DecimalItem to use the new getFieldSettings() method

Plus Drupal\number\Plugin\field\field_type\DecimalItem::preSave can use the new getFieldSetting() method

Comment #10

yched CreditAttribution: yched commented 30 July 2013 at 13:43

Status:

Needs work

» Needs review

File	Size
field_types-FieldDefinitionInterface-2051177-10.patch	9.18 KB

Yes, 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 commented 31 July 2013 at 15:05

Status:

Needs review

» Reviewed & tested by the community

One 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 .