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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
6.67 KB

Patch

yched’s picture

Issue summary: View changes

fix code sample

yched’s picture

Issue summary: View changes

link to D7 API

swentel’s picture

swentel’s picture

My guess is that we'll also have to add this to number and datetime as they've been converted already as well.

Status: Needs review » Needs work

The last submitted patch, field-settingsForm-hasData-2051157-1.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
8.08 KB

Rerolled, and number and date need to be updated as well.

yched’s picture

Cool! RTBC anyone ?

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, is ok :)

swentel’s picture

Important, 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)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Approved API change

Committed 7833e61 and pushed to 8.x. Thanks!

andypost’s picture

Issue tags: -Approved API change

Suppose this require change notice! Also the change is awesome!

swentel’s picture

No, change notice is for field typed data change notice.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

existing code will break - interfaces...