Coming from #2015691: Convert field type to typed data plugin for number module - #13 to #15.
We're trying to make FieldItem classes as agnostic as possible about configurable fields / base fields, and thus have them do all of their job using striclty FieldDefinitionInterface.
One issue is the settingsForm() method. Typically you need to check $field->hasData() in there and disable a couple form elements if TRUE. But hasData() is not part of FieldDefinitionInterface, and exists only for configurable fields.
No "real" problem in practice, since the fact that settingsForm() is being called means that the fields *is* a configurable field.
But strictly speaking the resulting code is a bit sloppy.
I'm not sure we need to add hasData() to FieldDefinitionInterface. Best way IMO is to let the caller (Field UI) call $field->hasData() and pass the result as an extra param to $item->settingsForm():
- An explicit param improves awareness about "i should watch this and set #access to FALSE where applicable"
- it prevents people from doing '#disabled' => $field->hasData() separately on each element (hasData() is one EFQ per call)
- that's how it worked in D7 (hook_field_settings_form), not sure why we (I) reverted that.
Comment | File | Size | Author |
---|---|---|---|
#5 | field-settingsForm-hasData-2051157-5.patch | 8.08 KB | swentel |
#5 | interdiff.txt | 2.55 KB | swentel |
#1 | field-settingsForm-hasData-2051157-1.patch | 6.67 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch
Comment #1.0
yched CreditAttribution: yched commentedfix code sample
Comment #1.1
yched CreditAttribution: yched commentedlink to D7 API
Comment #2
swentel CreditAttribution: swentel commented#1: field-settingsForm-hasData-2051157-1.patch queued for re-testing.
Comment #3
swentel CreditAttribution: swentel commentedMy guess is that we'll also have to add this to number and datetime as they've been converted already as well.
Comment #5
swentel CreditAttribution: swentel commentedRerolled, and number and date need to be updated as well.
Comment #6
yched CreditAttribution: yched commentedCool! RTBC anyone ?
Comment #7
aspilicious CreditAttribution: aspilicious commentedYeah, is ok :)
Comment #8
swentel CreditAttribution: swentel commentedImportant, if this one goes in first, #2015701: Convert field type to typed data plugin for entity reference module needs work, and vica versa (note that entity reference conversion is a bit more important and this one easier to reroll)
Comment #9
alexpottCommitted 7833e61 and pushed to 8.x. Thanks!
Comment #10
andypostSuppose this require change notice! Also the change is awesome!
Comment #11
swentel CreditAttribution: swentel commentedNo, change notice is for field typed data change notice.
Comment #12.0
(not verified) CreditAttribution: commentedexisting code will break - interfaces...