Remove the "configurable" flag on field types

If we do this, then we have to kill the Configurable*Interface interfaces and classes too and merge whatever they do up into their parent. Otherwise you could create a non-configurable field without no_ui and it would blow up.

If we do this, then we have to kill the Configurable*Interface interfaces and classes too and merge whatever they do up into their parent. Otherwise you could create a non-configurable field without no_ui and it would blow up.

I had a patch for this sitting on my desktop for quite a while so I opened an issue for it: #2192259: Remove ConfigField.. Item and List classes + interfaces

It's not unused, so let's remove that from the title.

So, I know there was a discussion on whether 'no_ui' and 'configurable' were redundant, but I can't really remember the arguments why both were kept :-/

Other than that, I do feel a little nervous about removing 'configurable' before we have removed the separation between "field type that can be used for configurable fields" and "field types that can only be used for base fields". Can't really point a specific reason atm, so this might just be FUD...

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
   @@ -52,11 +52,12 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   -      if ($definition['configurable']) {
   -        $definition['list_class'] = '\Drupal\Core\Field\ConfigFieldItemList';
   +      // Do not allow to configure a default value if "no_ui" is TRUE.
   

   How is this code (or the 'no_ui' flag) related to the notion of "default value" ?

2. +++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
   @@ -117,6 +117,9 @@ public function getInstance(array $options) {
   +      if (empty($field_type_definition['default_formatter'])) {
   +        return NULL;
   +      }
   

   Why is this needed ?

3. +++ b/core/lib/Drupal/Core/Field/WidgetPluginManager.php
   @@ -103,6 +103,9 @@ public function getInstance(array $options) {
   +      if (empty($field_type_definition['default_widget'])) {
   +        return NULL;
   +      }
   

   Same, why ?

4. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php
   @@ -137,8 +138,8 @@ public function buildForm(array $form, array &$form_state, FieldInstanceInterfac
   +    // Configure a default value if field implements.
   

   Unfinished sentence

So, I know there was a discussion on whether 'no_ui' and 'configurable' were redundant, but I can't really remember the arguments why both were kept :-/

Quoting Berdir on IRC:

a) We still needed a way to have configurable fields that can only be added in code, that's what no_ui is for.
b) back when we made base fields use the @FieldType annotation and introduced configurable, base fields weren't ready to be used as configurable fields (no schema() etc). but amateescu's issue to drop the ConfigField* classes and interfaces it going to remove the final blocker there

So right, doing this is fine, but I still vaguely feel it would be best to do things in order here and wait for #2192259: Remove ConfigField.. Item and List classes + interfaces , but I'll defer to @amateescu.

Yes, waiting for that does make sense, agreed. Long dependency chain, but I think that's preferable in this case.

I agree with @yched and @Berdir, it doesn't really make sense to remove 'configurable' until we remove the classes in #2192259: Remove ConfigField.. Item and List classes + interfaces. Also, hook_field_info() in D7 defined default_formatter and default_widget as non-optional, and I think we should stick with that (e.g. throw an exception if a field type doesn't declare them).

What I was wondering is if we should add a method on the field type manager to encapsulate *that* ? Or alternatively throw an exception for an field that does not have no_ui set but is missing a default widget or formatter?

I don't think we should make it required to have a widget and formatter for fields that can not be added in the UI, various core field types are not meant to be edited in the UI, for example the UUID (if anything, you could display it, but not edit and a read-only widget doesn't make much sense?).

1) Yep.
2) & 3) Yes, as mentioned above, I think we should keep a function to return field types that don't have no_ui set and a default widget and formatter.
4.) That would be cool but definitely a different issue.

What we are actually doing here is remove the "configurable" flag on field types, I think that's what the issue title should be saying. We'll keep a method on the field type manager (that's my suggestion anyway), just named differently. I'm not sure what.. getUiDefinitions() getDefinitionsWithUi() getAddableDefinitions() ? No idea...

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -102,7 +102,7 @@ function _content_translation_form_language_content_settings_form_alter(array &$
                // translation.
                // @todo Remove this special casing as soon as configurable and
                //   base field definitions are "unified".
   -            if ($definition->isConfigurable() && ($field = FieldService::fieldInfo()->getField($entity_type_id, $field_name))) {
   +            if ($field = FieldService::fieldInfo()->getField($entity_type_id, $field_name)) {
                  $instance = FieldService::fieldInfo()->getInstance($entity_type_id, $bundle, $field_name);
                  $form['settings'][$entity_type_id][$bundle]['fields'][$field_name] = array(
                    '#label' => $instance->getLabel(),
   

   Hm, you should be able to drop all this, because $definition *is* already the instance, and should therefore have the correct label.

2. +++ b/core/modules/field/field.deprecated.inc
   @@ -335,8 +335,7 @@ function field_attach_form(EntityInterface $entity, &$form, &$form_state, $langc
      $has_violations = FALSE;
      foreach ($entity as $field_name => $field) {
   -    $definition = $field->getFieldDefinition();
   -    if ($definition->isConfigurable() && (empty($options['field_name']) || $options['field_name'] == $field_name)) {
   +    if (empty($options['field_name']) || $options['field_name'] == $field_name) {
          $field_violations = $field->validate();
          if (count($field_violations)) {
            $has_violations = TRUE;
   

   I'd expect this check to go away in https://drupal.org/node/2095195, if not already, then it probably should.

Needs a re-roll due to email/number but looks great.

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -39,11 +39,11 @@ public function getDefaultInstanceSettings($type);
    
      /**
   -   * Gets the definition of all field types that are configurable.
   +   * Gets the definition of all field types that could be added via UI.
       *
   

   ... that can be added... I think.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EmailItem.php
   @@ -18,7 +18,7 @@
     *   label = @Translation("E-mail"),
     *   description = @Translation("An entity field containing an e-mail value."),
   - *   configurable = FALSE,
   + *   no_ui = TRUE,
     *   default_widget = "string",
   

   EmailItem has been merged, so no longer no_ui :)

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/FloatItem.php
   @@ -18,7 +18,7 @@
     *   label = @Translation("Float"),
     *   description = @Translation("An entity field containing an float value."),
   - *   configurable = FALSE
   + *   no_ui = TRUE
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php
   @@ -18,7 +18,7 @@
     *   label = @Translation("Integer"),
     *   description = @Translation("An entity field containing an integer value."),
   - *   configurable = FALSE
   + *   no_ui = TRUE
     * )
   

   Same.

4. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -102,7 +102,7 @@ function _content_translation_form_language_content_settings_form_alter(array &$
                // translation.
                // @todo Remove this special casing as soon as configurable and
                //   base field definitions are "unified".
   -            if ($definition->isConfigurable() && ($field = FieldService::fieldInfo()->getField($entity_type_id, $field_name))) {
   +            if ($field = FieldService::fieldInfo()->getField($entity_type_id, $field_name)) {
                  $instance = FieldService::fieldInfo()->getInstance($entity_type_id, $bundle, $field_name);
                  $form['settings'][$entity_type_id][$bundle]['fields'][$field_name] = array(
                    '#label' => $instance->getLabel(),
   

   $definition === $instance already, so you can remove this part completely.

5. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
   diff --git a/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
   index f325b85..c10a477 100644
   
   index f325b85..c10a477 100644
   --- a/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
   
   --- a/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
   +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
   
   +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
   +++ b/core/modules/text/lib/Drupal/text/Tests/TextWithSummaryItemTest.php
   @@ -127,33 +127,26 @@ function testProcessedCache() {
   
   @@ -127,33 +127,26 @@ function testProcessedCache() {
        // data.
   

   This was only necessar due to the entity cache change, it should no longer be needed. I'd suggest to remove it to avoid conflicts with the entity cache patch.

This still needed, because with a patch entities are cached with all fields

No they are not, they were at one point but we changed that back because it caused a bunch of other test fails. Try it, it should pass just fine without that change.

Agree on the @todo in content_translation, didn't see all of it, that's something for #2143291: Clarify handling of field translatability.

This looks ready to me, but a review from one of the field API maintainers would be nice.

Works for me.

I asked Berdir for some more information on this, since I wasn't totally grokking the purpose of this patch from the issue summary.

Basically, back when we had both "base" fields and "configurable" fields, this distinction made sense. But over time, we've settled on all fields deriving from the same "base" type. Now, "not-configurable" doesn't make sense as a separate distinction, because if all you want to do is prevent a field from being configured in the UI, you can use the existing no_ui property for that.

Therefore...

Committed and pushed to 8.x. Thanks!

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItem.php
@@ -21,7 +21,7 @@
- *   configurable = FALSE,
+ *   no_ui = TRUE,

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringLongItem.php
@@ -16,7 +16,7 @@
- *   configurable = FALSE
+ *   no_ui = FALSE

The second one looks like a mistake ?

We actually forgot a couple more

Committed and pushed that to 8.x, too.