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) { ... }

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Issue summary: View changes

minor rephrase

yched’s picture

Issue summary: View changes

issue link

plopesc’s picture

Status: Active » Needs review
FileSize
11.01 KB

First round.
Removing ConfigFieldInterface::getInstance() / ConfigFieldItemInterface::getInstance() methods and using getFieldDefinition().

With this patch, ConfigFieldInterface does not provide any method for now... Should then be removed?

Regards.

alexpott’s picture

Issue tags: +Approved API change

It is a stated aim that configurable fields and entity base fields should share as much functionality as possible. Therefore this change is approved.

yched’s picture

Issue tags: -Approved API change

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

yched’s picture

Issue tags: +Approved API change

er, crosspost...

Status: Needs review » Needs work

The last submitted patch, remove_getinstance-2074547-1.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
11.69 KB

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

effulgentsia’s picture

This 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:

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.php
@@ -128,7 +128,7 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
-    $field_definition = isset($form_state['instance']) ? $form_state['instance'] : $this->getInstance();
+    $field_definition = isset($form_state['instance']) ? $form_state['instance'] : $this->getFieldDefinition();

$form_state['instance'] should always be set here, so can we replace this with $instance = $form_state['instance']?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -129,7 +107,7 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
-      return $callback($this->getInstance()->getField(), $this->getInstance(), $has_data);
+      return $callback($this->getFieldDefinition()->getField(), $this->getFieldDefinition(), $has_data);
...
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -142,7 +120,7 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
-      return $callback($this->getInstance()->getField(), $this->getInstance(), $form_state);
+      return $callback($this->getFieldDefinition()->getField(), $this->getFieldDefinition(), $form_state);

Can we have a protected getInstance() function in this class that just returns getFieldDefinition(), and use it for the above?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/FieldType/ConfigEntityReferenceItemBase.php
@@ -161,7 +139,7 @@ public function getSettableOptions() {
-      return $callback($this->getInstance(), $entity);
+      return $callback($this->getFieldDefinition(), $entity);

This one's correct though, because this callback does actually expect a field definition, not an instance.

+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigField.php
@@ -42,12 +42,12 @@ public function validate() {
-    if (isset($legacy_errors[$this->getInstance()->getField()->id()][$langcode])) {
-      foreach ($legacy_errors[$this->getInstance()->getField()->id()][$langcode] as $delta => $item_errors) {
+    if (isset($legacy_errors[$this->getFieldDefinition()->getField()->id()][$langcode])) {
+      foreach ($legacy_errors[$this->getFieldDefinition()->getField()->id()][$langcode] as $delta => $item_errors) {

s/getFieldDefinition()->getField()->id()/getFieldDefinition()->getFieldName()/

+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigField.php
@@ -42,12 +42,12 @@ public function validate() {
-          $column = key($this->getInstance()->getField()->getColumns());
+          $column = key($this->getFieldDefinition()->getField()->getColumns());

This can be changed to something like $property_names = $this->getFieldDefinition()->getFieldPropertyNames(); $property_name = $property_names[0];.

+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigField.php
@@ -115,8 +115,8 @@ protected function legacyCallback($hook, $args = array()) {
-        $this->getInstance()->getField(),
-        $this->getInstance(),
+        $this->getFieldDefinition()->getField(),
+        $this->getFieldDefinition(),

Can we have a protected getInstance() function in this class that just returns getFieldDefinition(), and use it for the above?

+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigFieldItem.php
@@ -49,7 +49,7 @@ public function isEmpty() {
-    return empty($item) || $callback($item, $this->getInstance()->getField()->type);
+    return empty($item) || $callback($item, $this->getFieldDefinition()->getField()->type);

This can be getFieldDefinition()->getFieldType().

+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigFieldItem.php
@@ -59,7 +59,7 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
-      return $callback($this->getInstance()->getField(), $this->getInstance(), $has_data);
+      return $callback($this->getFieldDefinition()->getField(), $this->getFieldDefinition(), $has_data);
...
+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigFieldItem.php
@@ -69,7 +69,7 @@ public function settingsForm(array $form, array &$form_state, $has_data) {
-      return $callback($this->getInstance()->getField(), $this->getInstance(), $form_state);
+      return $callback($this->getFieldDefinition()->getField(), $this->getFieldDefinition(), $form_state);
...
+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigFieldItem.php
@@ -96,8 +96,8 @@ public function prepareCache() {
-        $this->getInstance()->getField(),
-        array($entity_id => $this->getInstance()),
+        $this->getFieldDefinition()->getField(),
+        array($entity_id => $this->getFieldDefinition()),

Can we have a protected getInstance() function in this class that just returns getFieldDefinition(), and use it for the above?

+++ b/core/modules/field/lib/Drupal/field/Plugin/field/field_type/LegacyConfigFieldItem.php
@@ -119,7 +119,7 @@ public function getSettableOptions() {
-      return $callback($this->getInstance(), $entity);
+      return $callback($this->getFieldDefinition(), $entity);

This one's correct, because this callback does expect a field definition.

yched’s picture

Thanks @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.

plopesc’s picture

Patch including @effulgentsia suggestions.

Included protected method getInstance() in ConfigEntityReferenceItemBase, LegacyConfigField and LegacyConfigFieldItem classes.

Regards.

Status: Needs review » Needs work

The last submitted patch, remove_getinstance-2074547-9.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
FileSize
741 bytes
13.1 KB

Re-rolling after #2056405: Let field types provide their support for "default values" and associated input UI added another call to getInstance().

Regards.

effulgentsia’s picture

Thanks! This has a few more cleanups.

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' ?)

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.

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: +API change, +Field API, +Entity Field API, +Approved API change

The last submitted patch, remove_getinstance-2074547-12.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
15.73 KB

Straight reroll.

yched’s picture

re: 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 :

  1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.php
    @@ -166,7 +166,7 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    -      '#default_value' => $field_definition->getFieldSetting('handler') ?: 'default',
    +      '#default_value' => $instance->getFieldSetting('handler'),
    

    this should be $this->getFieldSetting('handler') ?

  2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.php
    @@ -186,8 +186,8 @@ public function instanceSettingsForm(array $form, array &$form_state) {
    +    $handler = \Drupal::service('plugin.manager.entity_reference.selection')->getSelectionHandler($instance);
    +    $form['handler']['handler_settings'] += $handler->settingsForm($instance);
    

    Both methods are type hinted to receive a FieldDefinitionInterface, so we could just pass $this->getFieldDefinition() instead of $instance ?

Other than that, yup, RTBC :-)

effulgentsia’s picture

But in fact that code should not need that $form_state['instance'], there are much better ways to do this now

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

Status: Needs review » Needs work

The last submitted patch, remove_getinstance-2074547-17.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
15.73 KB

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

yched’s picture

Status: Needs review » Reviewed & tested by the community

Meant to do this

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

yched’s picture

Awesome! Thanks a lot @plopesc & @effulgentsia!

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

Anonymous’s picture

Issue summary: View changes

API changes