Remove ConfigField.. Item and List classes + interfaces

I thought configurable defaults to TRUE anyway? Or is supposed to...

Overall +1 to get rid of this classes & interfaces, the only difference between configurable and "none-configurable" that last does not provides default widget and formatter that was fixed in #2191709-11: Remove the "configurable" flag on field types

Why you wanna leave "configurable", I think "no_ui" better describes what this for

+++ b/core/modules/telephone/lib/Drupal/telephone/Plugin/Field/FieldType/TelephoneItem.php
@@ -18,11 +18,12 @@
+ *   configurable = TRUE,

+++ b/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItem.php
@@ -16,6 +16,7 @@
+ *   configurable = TRUE,

This makes developers to write more, but most of time contrib will provide a configurable fields, and TRUE is default in annotation

1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
   @@ -167,7 +166,7 @@ protected function loadFieldItems(array $entities) {
   -              if ($items instanceof ConfigFieldItemListInterface && !$items->isEmpty()) {
   +              if ($items->getFieldDefinition()->isConfigurable() && !$items->isEmpty()) {
   

   Maybe better to find that items produced from "field instance"? because relying on some annotation in storage controller is bad idea

2. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -48,6 +57,11 @@ public function __construct(DataDefinitionInterface $definition, $name = NULL, T
   +    // Definition can be the field config or field instance.
   +    if ($definition instanceof FieldInstanceInterface) {
   +      $this->instance = $definition;
   +    }
   

   maybe just field instance? as variable named...
   Also isConfigurable could simply check that $instance initialized...

3. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -75,7 +89,41 @@ public function getLangcode() {
   +    // Configurable fields have the field_config entity injected as definition,
   +    // but we want to return the more specific field instance here.
   ...
   +    if ($this->definition->isConfigurable() && !isset($this->instance)) {
   

   this code already does that

4. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -75,7 +89,41 @@ public function getLangcode() {
   +    $cardinality = $this->getFieldDefinition()->getCardinality();
   

   suppose this needs test. base fields should have cardinality 1

5. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -278,4 +326,80 @@ protected function delegateMethod($method) {
   +  public function defaultValuesForm(array &$form, array &$form_state) {
   ...
   +      $widget = $this->defaultValueWidget($form_state);
   

   what fallback could be done for fields that have no default widget

6. +++ b/core/modules/email/email.module
   @@ -32,8 +32,6 @@ function email_help($path, $arg) {
      $info['email']['default_widget'] = 'email_default';
      if (\Drupal::moduleHandler()->moduleExists('text')) {
        $info['email']['default_formatter'] = 'text_plain';
   

   seems need separate issue - wtf email default formater from text module?

The test fails are related to serialization, somehow the entity ends up with two langcode field items, one of them being empty.

No idea why that would be caused by this, so I asked @damiankloip to have a look.

1. +++ b/core/lib/Drupal/Core/Entity/FieldableEntityStorageControllerBase.php
   @@ -167,7 +166,7 @@ protected function loadFieldItems(array $entities) {
              foreach ($entity->getTranslationLanguages() as $langcode => $language) {
                $translation = $entity->getTranslation($langcode);
                foreach ($translation as $field_name => $items) {
   -              if ($items instanceof ConfigFieldItemListInterface && !$items->isEmpty()) {
   +              if ($items->getFieldDefinition()->isConfigurable() && !$items->isEmpty()) {
                    foreach ($items as $delta => $item) {
                      // If the field item needs to prepare the cache data, call the
   

   That limitation is removed in #597236: Add entity caching to core, so I'm fine with doing whatever needs to be done to make it work and then move on.

2. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -278,4 +278,101 @@ protected function delegateMethod($method) {
   +    // Check that the number of values doesn't exceed the field cardinality. For
   +    // form submitted values, this can only happen with 'multiple value'
   +    // widgets.
   +    $cardinality = $this->getFieldDefinition()->getCardinality();
   +    if ($cardinality != FieldDefinitionInterface::CARDINALITY_UNLIMITED) {
   

   Ah, this is the what caused the rest tests to fail, we previously didn't validate the cardinality of base fields.. nice.

3. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/EntityNormalizer.php
   @@ -118,6 +118,8 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
        }
    
   +    unset($data['langcode']);
   +
        $entity = entity_create($typed_data_ids['entity_type'], array('langcode' => $langcode, 'type' => $typed_data_ids['bundle']));
    
   

   Can we move this within the "if (isset($data['langcode']) {" above, where it is used to set $langcode? That would be much more self-explaining than down here.

Other than that, I think this is ready to fly :)

To respond to @andypost's review above:

1. See my point above, will no longer be needed after that issue, so really doesn't matter what we chose.
2. This code is gone.
3. This too.
4. We're just moving code around here and rest.module was so nice to confirm that it works ;)
5. This I'm not sure about, can we throw an exception for now? I would simply say that a field that can be created through the UI must have a default_widget.
6. Unrelated.

Agreed, that makes sense. Added a comment too (for future selves :)).

That's fine, we still have 5. from @andypost's review, but I think we can deal with that in #2191709: Remove the "configurable" flag on field types?

However, based on a short discussions with @Crell, I got reminded that we in fact have a base field that has a cardinality > 1 and that is the user.roles field.

Right now, it's not possible to configure this for base fields, it's hardcoded to 1. That means that the validation will fail when trying to validate a user with more than one assigned user roles.

That's not happening right now, which means that UserValidationTest is missing test coverage for multiple roles. As this would introduce a bug and break rest for users with more than one role, I think it's blocking this issue, which means that we a) need test coverage for validating a user with more than one role (which should be valid) and add a setCardinality() to FieldDefinition.

+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -218,4 +218,18 @@ public function delete() { }
+  public function settingsForm(array $form, array &$form_state, $has_data) {
+    return array();

+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -196,4 +196,44 @@ public function delete();
+  public function settingsForm(array $form, array &$form_state, $has_data);

From where does the caller know whether it should present the form or skip it? Would it make sense to put this into an optional interface instead, e.g. FieldSettingsFormInterface or so?

Or is knowing this not important anyway?

Throw an exception like this? If it's wrong, sorry, I don;t have much day to day business in this area of the code :)

I don't have time to write those tests now, I might in the next couple of days unless someone else gets there first.

@fago #21 : the caller (field_ui) calls the "form settings" methods only for fields where the settings can actually be saved - I.e. config fields.

The goal is that any field type can be used for config fields, so it's not up to the field type class to decide. Moving the methods to a separate interface (+ then with a base class ?) would be ... exactly what we have now :)

Suppose the decision about "configurability" could be done per entity by some method isBaseField()
otoh field_ui could override base fields definition to store instance settings and default values per bundle

I think the proper place to throw an exception is in the definition process phase of the field type plugin discovery, when we can check if the field type specifies a default widget or not.

This applies to formatters too.

I disagree here, field type plugin manager should find and instantiate plugins, and corresponding managers (formatter and widget) or glue code should care about default values.
In #2191709: Remove the "configurable" flag on field types I specifically returns NULL when no default widget or formatter found in getInstance().
Probably we could introduce some kind of BrokenWidget and BrokenFormatter plugin to throw notice when formatter or widget not found

We don't support to handle storage for multivalue base fields, but we have to handle the user role base field *somehow*. Which we already do.

However, we need a way to make set the correct cardinality there, otherwise it will be impossible to save users with more than one role through rest. And it is possible to do so right now, so IMHO, we need to fix that here.

I'm fine with moving the default widget problem to another issue.

Discussed with @Berdir at IRC - let's add FieldDefinition::setCardinality() with user role test
Also filed #2213727: Allow field types without default widget or formatter

So I think that one is RTBC

PS: removed @todo points to closed issue

@amateescu Do you have another idea to test cardinality?
I think it's a good usage of the api addition that have a great test coverage

Trying to explain:

Yes, we should *not* make it an ER field here, agree with @amatescu on that as mentioned below too. We do need cardinality however:

This patch moves the cardinality validation from ConfigFieldItemBase to FieldItemBase. That means the cardinality is now (correctly) validated for base fields, and they must only have a single item. This is why the rest tests failed above as it added two languages, one being empty (that was then filtered out before saving).

We currently did not have any test cases that covered this, but the user roles field *has* unlimited cardinality, and we need a way to define that or it would be impossible to validate users with multiple roles. This is something introduced in this issue, so we have to fix it here.

Now, yes, your snippet does not work, but that has nothing to do with the cardinality. Passing in a single role would break as well. The issue to fix that is #1751274: Do not query user_roles directly and it should also work for multiple cardinality then.

We have to support cardinality for base fields, that does not mean that we need to support storage for them out of the box.

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -178,6 +178,23 @@ public function getCardinality() {
   +   *
   +   * @param integer $cardinality
   +   *  The field cardinality.
   +   *
   

   int

2. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -178,6 +178,23 @@ public function getCardinality() {
   +   * @return static
   +   *   The object itself for chaining.
   

   I think this should be @return $this now, without description?

3. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -178,6 +178,23 @@ public function getCardinality() {
   +   */
   +  public function setCardinality($cardinality) {
   +    $this->definition['cardinality'] = $cardinality;
   +    return $this;
   +  }
   

   It would be nice to extend FieldDefinitionTest::testFieldCardinality(), it already has a todo for this :)

4. +++ b/core/modules/user/lib/Drupal/user/Entity/User.php
   @@ -512,9 +513,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        // @todo Convert this to entity_reference_field, see
        // https://drupal.org/node/2044859.
   -    $fields['roles'] = FieldDefinition::create('string')
   +    $fields['roles'] = FieldDefinition::create('entity_reference')
          ->setLabel(t('Roles'))
   

   Let's not do this here, this has nothing to do with this issue. We should only set the cardinality here.

5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserValidationTest.php
   @@ -38,6 +38,7 @@ public function setUp() {
        $this->installSchema('user', array('users'));
        $this->installSchema('system', array('sequences'));
   +    $this->installConfig(array('user'));
   

   Why do we need the config?

6. +++ b/core/modules/user/lib/Drupal/user/Tests/UserValidationTest.php
   @@ -142,8 +143,22 @@ function testValidation() {
    
   -    // @todo Test user role validation once https://drupal.org/node/2015701 got
   -    // committed.
   +    $role1 = entity_create('user_role', array(
   +      'id' => 'role1',
   +      'label' => 'Label 1'
   +    ));
   

   Let's keep the @todo, we still need to validate that only valid roles are assigned once we maked it an ER field and so on.

1-3) Fixed
4) reverted
5) reverted, this is needed to import roles (anonymous and authorized)
6) reverted, updated @todo with proper issue #

Add test for user roles cardinality

Thanks, I think that looks good now!

If my explanations didn't convince someone, speak up now :)

+++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
@@ -178,6 +178,22 @@ public function getCardinality() {
+   *
+   * @param int  $cardinality
+   *  The field cardinality.
+   *

Two spaces after int, but I think we can clean that up on commit?

Re-roll to remove 2 spaces.

If a field needs to store multiple values, it shouldn't be a base field

I think we can't be so strict here, because contrib could alter any definition, so fieldList is responsive about storage

Drupal\Core\Field\ConfigFieldItemList<code> still exists and implements the removed interface <code>ConfigFieldItemListInterface

EDIT ConfigFieldItemList removal

Re-roll after #2169361: Convert COMMENT_HIDDEN & COMMENT_CLOSED & COMMENT_OPEN to a constant on the comment field interface

44: 2192259-no-cfield-44.patch queued for re-testing.

And one more merge error

Back to RTBC. This looks like a great simplification, awesome! :-)

Committed 7ce95f3 and pushed to 8.x. Thanks!

Existing change notices to update:

• https://drupal.org/node/2064123
• https://drupal.org/node/2111927
• https://drupal.org/node/2076489
• https://drupal.org/node/2101747

As I see all this change notices are outdated, maybe there's some issue to re-work them?

yched_afk dances at the commit :) Awesome!

Updated https://drupal.org/node/2101747 & https://drupal.org/node/2076489 & https://drupal.org/node/2111927
https://drupal.org/node/2064123 needs serious changes, so better to wait for #2191709: Remove the "configurable" flag on field types