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

Files: 
CommentFileSizeAuthor
#19 remove_getinstance-2074547-18.patch15.73 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]
#17 remove_getinstance-2074547-17.patch15.59 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,443 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 interdiff.txt1.82 KBeffulgentsia
#15 remove_getinstance-2074547-15.patch15.73 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]
#12 remove_getinstance-2074547-12.patch15.73 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove_getinstance-2074547-12_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 interdiff.txt8.73 KBeffulgentsia
#11 remove_getinstance-2074547-11.patch13.1 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,798 pass(es).
[ View ]
#11 interdiff.txt741 bytesplopesc
#9 remove_getinstance-2074547-9.patch12.77 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 58,199 pass(es), 26 fail(s), and 23 exception(s).
[ View ]
#9 interdiff.txt7.94 KBplopesc
#6 remove_getinstance-2074547-6.patch11.69 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,489 pass(es).
[ View ]
#6 interdiff.txt1.48 KBplopesc
#1 remove_getinstance-2074547-1.patch11.01 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] 57,011 pass(es), 230 fail(s), and 130 exception(s).
[ View ]

Comments

Issue summary:View changes

minor rephrase

Issue summary:View changes

issue link

Status:Active» Needs review
StatusFileSize
new11.01 KB
FAILED: [[SimpleTest]]: [MySQL] 57,011 pass(es), 230 fail(s), and 130 exception(s).
[ View ]

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.

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.

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

Issue tags:+Approved API change

er, crosspost...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.48 KB
new11.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,489 pass(es).
[ View ]

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.

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.

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.

StatusFileSize
new7.94 KB
new12.77 KB
FAILED: [[SimpleTest]]: [MySQL] 58,199 pass(es), 26 fail(s), and 23 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new741 bytes
new13.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,798 pass(es).
[ View ]

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

Regards.

StatusFileSize
new8.73 KB
new15.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove_getinstance-2074547-12_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new15.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,433 pass(es).
[ View ]

Straight reroll.

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

StatusFileSize
new1.82 KB
new15.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,443 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new15.73 KB
PASSED: [[SimpleTest]]: [MySQL] 58,460 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Meant to do this

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

Awesome! Thanks a lot @plopesc & @effulgentsia!

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

Issue summary:View changes

API changes