It was added in the rush of the home stretch for the "field types as plugins" patch, when figuring out how to merge with the FieldDefinitionInterface that went in in parallel.
But as we've seen in the various "convert D7 field types to plugins" issues since then, its existence makes it very tempting to use instead of getFieldDefinition(), de facto making the class unusable for base fields. We don't want to end up with almost-duplicate field types, one that works for base fields, one that works for configurable fields - see also #2052135: Determine how to deal with field types used in base fields in core entities...
The same "field type" classes should be usable for base fields and configurable fields alike, and do their work using getFieldDefinition() / FieldDefinitionInterface only, without assuming that it is a "configurable FieldInstance".
API change
- ConfigFieldInterface::getInstance() / ConfigFieldItemInterface::getInstance() are removed.
- Field type classes need to use the existing getFieldDefinition() instead, and work on those as FieldDefinitionInterface objects, not FieldInstance / FieldInstanceInterface objetcs.
- If needed, special casing on "if configurable field" can be done the regular OO way: if ($this->getFieldDefinition() instanceof FieldInstanceInterface) { ... }
Comment | File | Size | Author |
---|---|---|---|
#19 | remove_getinstance-2074547-18.patch | 15.73 KB | yched |
#17 | remove_getinstance-2074547-17.patch | 15.59 KB | effulgentsia |
#17 | interdiff.txt | 1.82 KB | effulgentsia |
#15 | remove_getinstance-2074547-15.patch | 15.73 KB | effulgentsia |
#12 | remove_getinstance-2074547-12.patch | 15.73 KB | effulgentsia |
Comments
Comment #0.0
yched CreditAttribution: yched commentedminor rephrase
Comment #0.1
yched CreditAttribution: yched commentedissue link
Comment #1
plopescFirst round.
Removing
ConfigFieldInterface::getInstance() / ConfigFieldItemInterface::getInstance()
methods and usinggetFieldDefinition()
.With this patch,
ConfigFieldInterface
does not provide any method for now... Should then be removed?Regards.
Comment #2
alexpottIt is a stated aim that configurable fields and entity base fields should share as much functionality as possible. Therefore this change is approved.
Comment #3
yched CreditAttribution: yched commented#2056405: Let field types provide their support for "default values" and associated input UI adds methods in ConfigFieldInterface ;-), so I'd say keep it around for now...
Comment #4
yched CreditAttribution: yched commenteder, crosspost...
Comment #6
plopescTests fails because of "crosscommitting" with #1758622: Provide the options list of an entity field, which added two calls to the now removed
getInstance()
method.Re-rolling again, it should be green now.
Regards.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedThis looks good, except I'd like us to try to always use the result of getFieldDefinition() as a field definition, and not make assumptions that it can be treated as a FieldInstanceInterface. With that in mind, can we do the following:
$form_state['instance'] should always be set here, so can we replace this with
$instance = $form_state['instance']
?Can we have a protected getInstance() function in this class that just returns getFieldDefinition(), and use it for the above?
This one's correct though, because this callback does actually expect a field definition, not an instance.
s/getFieldDefinition()->getField()->id()/getFieldDefinition()->getFieldName()/
This can be changed to something like
$property_names = $this->getFieldDefinition()->getFieldPropertyNames(); $property_name = $property_names[0];
.Can we have a protected getInstance() function in this class that just returns getFieldDefinition(), and use it for the above?
This can be getFieldDefinition()->getFieldType().
Can we have a protected getInstance() function in this class that just returns getFieldDefinition(), and use it for the above?
This one's correct, because this callback does expect a field definition.
Comment #8
yched CreditAttribution: yched commentedThanks @plopesc.
Agreed with the remarks in #7.
Additionally, we would need @amateescu's feedback about the impact & consequences on the code in entity_reference (like - does $form_state['instance'] really need to be called 'instance' ?)
Not too worried about what happens in LegacyConfigField*, those classes are temporary BC and will disappear at some point. A protected getInstance() helper method sounds fine.
Comment #9
plopescPatch including @effulgentsia suggestions.
Included protected method getInstance() in ConfigEntityReferenceItemBase, LegacyConfigField and LegacyConfigFieldItem classes.
Regards.
Comment #11
plopescRe-rolling after #2056405: Let field types provide their support for "default values" and associated input UI added another call to getInstance().
Regards.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedThanks! This has a few more cleanups.
That's decided in FieldInstanceEditForm::buildForm(). Out of scope for this issue to change. I also think it makes sense: we're in the field instance settings form, so it's legitimate to act on a field instance.
I think this is good to go, but I guess I'm not eligible to RTBC it any more.
Comment #13
effulgentsia CreditAttribution: effulgentsia commented#12: remove_getinstance-2074547-12.patch queued for re-testing.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #16
yched CreditAttribution: yched commentedre: ConfigurableEntityReferenceItem::instanceSettingsForm() / $form_state['instance']:
Yup, silly me, I didn't read the code properly, I thought that was an entity_reference thing, but it's just the $form_state['instance'] set by FieldInstanceEditForm.
But in fact that code should not need that $form_state['instance'], there are much better ways to do this now, and the $instance var should just go away :
this should be $this->getFieldSetting('handler') ?
Both methods are type hinted to receive a FieldDefinitionInterface, so we could just pass $this->getFieldDefinition() instead of $instance ?
Other than that, yup, RTBC :-)
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedMy hunch is that $form_state['instance'] is being used, because $this->getFieldDefinition() returns the field instance that's loaded from whatever has been last saved, whereas $form_state['instance'] can contain some in-progress changes that haven't been saved to the config entity yet. But, let's see if there's any test that actually fails due to that.
Comment #19
yched CreditAttribution: yched commented@effulgentsia: you're very probably right, I forgot entity_ref did ajax stuff in this form.
This could still be cleanup up with a minimal effort in FieldInstanceEditForm, but I'll open a separate issue.
Meanwhile, #15 is RTBC.
Re-uploading it here just for clarity.
Comment #20
yched CreditAttribution: yched commentedMeant to do this
Comment #21
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #22
yched CreditAttribution: yched commentedAwesome! Thanks a lot @plopesc & @effulgentsia!
Comment #23.0
(not verified) CreditAttribution: commentedAPI changes