Updated: Comment #75

Problem/Motivation/Current situation

Vision: Unify base and configurable fields and their handling.

For historical reasons, base field types (e.g. StringItem, IntegerItem, EntityReferenceItem) are directly exposed as Typed Data (with ID's like string_field, integer_field, entity_reference_field).

Configurable fields were ported to the @FieldType Plugin type. They use plugin ID's like text, email, entity_reference. They are then also exposed as typed data plugins as derivatives and are prefixed with field_item:, so e.g. field_item:text, field_item:entity_reference.

Proposed resolution

To be able to instantiate all field types using the field type plugin manager, base fields need to be exposed as @FieldType plugins too. To keep them apart, they have a configurable = TRUE/FALSE flag, that indicates if they *can* be used to create configurable fields, but they can also be used as base fields. Classes that want to support configurable fields need to implement ConfigFieldIteminterface (and extend from the corresponding base class), they also automatically get the corresponding ConfigFieldItemList list class.

This is the what this patch does, All *Item.php classes are moved from \Drupal\Core\Entity\Plugin\DataType to ..Plugin\field\field_type.

That results in a number of smaller and bigger problems:

- We get naming conflicts because. Notably the email and entity_reference field types currently have a simple version that's in Core and have modules that provide widgets/formatters and so on. The patch solves that problem in two different ways:
-- entity_reference.module alters the field type definition and replaces it using it's own implementation. This means that enabling entity_reference.module results in a different runtime class being used for base fields like $node->uid.
-- Same for EmailItem/email.module.
- We need to prevent non-configurable fields from showing up in the field UI. This patch adds a new method, getConfigurableDefinitions(), to the field type plugin manager to make those easily accessible.
- Base field names change from *_field to field_item:*. To avoid having to change ~10 baseFieldDefinition() implementation, this patches introduces a simply mapping for those names. This is temporary until #2047229: Make use of classes for entity field and data definitions lands, which will replace those magic naming patterns with speaking methods like $definition->getFieldType(). The mapping layer means that we don't have to change those methods twice.
- The fact that the configurable entity reference implementation is used resulted in some interesting test fails because they considered references to uid as a new entity and tried to auto-save it. This has been prevented by overriding isNew() for users and return FALSE for uid 0, which is a correct fix anyway.
- CommentItem changes: > It has field properties defined as entity_reference_field. This is completely bogus, a field item can't have field types as properties. It only works because we never instantiate it thanks to the lazy-loading logic in field items.
- Last, validation constraint messages are currently not dynamic, so configurable field type classes create a dynamic constraint that allows them to add a validation message during runtime. The result is that we suddently have two validation fails when an e-mail is too long in a test. This will be unified in https://drupal.org/node/2012690. It's not a new behavior, it would have already happened for configurable fields, now it also happens for base fields.

Remaining tasks

See follow-up issues references above.

We also need to continue with the approach discussed in Prague, to use the field type plugin manager for instantiating field item classes and decouble field item classes from TypedDataInterface. This is a blocker for additional work on that.

User interface changes

None.

API changes

As outlined above, the plugin ID's change.

Original report by @fago

#1969728: Implement Field API "field types" as TypedData Plugins defines a new field_type plugin, once it's in we can convert entity field types over to make use of it. However for that, we need to make field.module respect the "configurable" flag such that it does not deal with non-configurable fields.

Files: 
CommentFileSizeAuthor
#78 convert_entity_field_type-77.patch48.13 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,984 pass(es).
[ View ]
#78 convert_entity_field_type-77-interdiff.txt976 bytesBerdir
#76 convert_entity_field_type-75.patch48.1 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,637 pass(es).
[ View ]
#73 convert_entity_field_type-73.patch48.42 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_entity_field_type-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#73 convert_entity_field_type-73-interdiff.txt7.99 KBBerdir
#71 convert_entity_field_type-71.patch50.39 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 58,610 pass(es).
[ View ]
#71 interdiff-69-71.txt976 bytessmiletrl
#69 d8_entity_field_type-2023563-68.patch50.42 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,746 pass(es), 69 fail(s), and 27 exception(s).
[ View ]
#69 d8_entity_field_type-2023563-68-interdiff.txt4.18 KBBerdir
#65 d8_entity_field_type-2023563-65.patch49.06 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,502 pass(es).
[ View ]
#61 d8_entity_field_type-2023563-61.patch49.05 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type-2023563-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#61 d8_entity_field_type-2023563-61-interdiff.txt1.81 KBBerdir
#57 d8_entity_field_type-2023563-57.patch48.71 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,787 pass(es).
[ View ]
#57 d8_entity_field_type-2023563-57-interdiff.txt1.55 KBBerdir
#55 d8_entity_field_type-2023563-55-test.patch45.05 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,454 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#55 d8_entity_field_type-2023563-55.patch48.78 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,781 pass(es).
[ View ]
#55 d8_entity_field_type-2023563-55-interdiff.txt10.46 KBBerdir
#54 no_ui-interdiff.txt7.96 KBsmiletrl
#47 d8_entity_field_type-2023563-47.patch42.11 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]
#47 d8_entity_field_type-2023563-47-interdiff.txt7.14 KBBerdir
#43 d8_entity_field_type-2023563-43.patch42.25 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,380 pass(es).
[ View ]
#43 d8_entity_field_type-2023563-43-interdiff.txt1.24 KBBerdir
#41 d8_entity_field_type-2023563-41.patch41.94 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,086 pass(es), 20 fail(s), and 37 exception(s).
[ View ]
#41 d8_entity_field_type-2023563-41-interdiff.txt8.89 KBBerdir
#37 d8_entity_field_type-2023563-37.patch39.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,309 pass(es), 20 fail(s), and 1 exception(s).
[ View ]
#37 d8_entity_field_type-2023563-37-interdiff.txt3.04 KBBerdir
#33 d8_entity_field_type-2023563-33.patch36.94 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,484 pass(es), 23 fail(s), and 6 exception(s).
[ View ]
#33 d8_entity_field_type-2023563-33-interdiff.txt1.05 KBBerdir
#31 d8_entity_field_type-2023563-30.patch36.92 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 105 fail(s), and 33 exception(s).
[ View ]
#31 d8_entity_field_type-2023563-30-interdiff.txt7.22 KBBerdir
#30 d8_entity_field_type-2023563-28.patch30.34 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#30 d8_entity_field_type-2023563-28-interdiff.txt3.19 KBBerdir
#28 d8_entity_field_type-2023563-28.patch30.34 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,630 pass(es), 182 fail(s), and 56 exception(s).
[ View ]
#28 d8_entity_field_type-2023563-28-interdiff.txt3.19 KBBerdir
#26 d8_entity_field_type-2023563-26.patch27.15 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 54,186 pass(es), 570 fail(s), and 263 exception(s).
[ View ]
#26 d8_entity_field_type-2023563-26-interdiff.txt1.04 KBBerdir
#25 d8_entity_field_type-2023563-25.patch25.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type-2023563-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 d8_entity_field_type-2023563-25-interdiff.txt5.69 KBBerdir
#23 d8_entity_field_type-2023563-23.patch23.65 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,762 pass(es), 149 fail(s), and 54 exception(s).
[ View ]
#22 d8_entity_field_type-2023563-22.patch11.89 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,898 pass(es).
[ View ]
#22 d8_entity_field_type-2023563-22-interdiff.txt4.5 KBBerdir
#20 d8_entity_field_type-2023563-20.patch16.38 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 instantiate_FieldItem_via_FieldTypePluginManager.patch792 bytessmiletrl
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 d8_entity_field_type-2023563-9.patch18.27 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type-2023563-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 interdiff-4-9.txt1.72 KBsmiletrl
#4 d8_entity_field_type.patch17.42 KBfago
PASSED: [[SimpleTest]]: [MySQL] 57,478 pass(es).
[ View ]
#3 d8_entity_field_type.patch16.77 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Postponed
Issue tags:+Entity Field API

Status:Postponed» Active

Status:Active» Needs review
StatusFileSize
new16.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok, here is a first patch doing the general changes necessary + converting the entity language field as a first test.

StatusFileSize
new17.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,478 pass(es).
[ View ]

Previous patch was missing a git commit, attached is the right one - sry.

+++ b/core/lib/Drupal/Core/Entity/Plugin/field/field_type/LanguageItem.php
@@ -0,0 +1,111 @@
+ *   module = "system",

Note that this is not nice, but should be fixed automatically with #2041423: Rely on 'provider' instead of 'module' for Field plugin types.

Could we move the array_filter logic in field_info_field_types() to a getConfigurableTypes() (or something) method in the manager ?

Good point, but it's not getting a type plugin (instance) but a definition - so I guess that should be reflected in the naming? Or maybe, we just extend getDefinitions with an optional $configurable filter?

<?php
public function getDefinitions($configurable = NULL) { }
?>

Then, I guess field_info_field_types() should be deprecated in favour of using the manager directly?

Build on getDefinitions() : yup, makes sense.
field_info_*_types() functions are totally up for deprecation, yes.

Then, I guess field_info_field_types() should be deprecated in favour of using the manager directly

Patch in #2047983: Mark field_info_*_types() / field_info_*_settings() functions as deprecated.

StatusFileSize
new1.72 KB
new18.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type-2023563-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled patch at #4. Also applied changes for last comments.

+ *   list_class = "\Drupal\Core\Entity\Field\Field",
+ *   constraints = {
+ *     "ComplexData" = {
+ *       "value" = {"Length" = {"max" = 12}}
+ *     }

hmm, should this be converted to public function getConstraints() for item class too?

Also, maybe add doc like Overrides \Drupal\Core\Plugin\DefaultPluginManager::getDefinitions(). for getDefinitions($configurable = NULL)?

Next step is to convert all other item datatype under namespace Drupal\Core\Entity\Plugin\DataType? Are these item class used by other systems, not only used as base fields?

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-9.patch, failed testing.

Is there a way this can let us get rid of "FieldItem classes are exposed as FieldType plugins, then derived as DataType plugins, and instantiated through TypedDataManager" ?
As we talked about in Portland,
- this is really confusing (FieldItem::getPluginId() returns the id as a DataType rather than the id defined at the top of the class)
- this means duplicated static caches in FieldTypePluginManager & TypedDataManager

Hmm, not sure about this:
(FieldItem::getPluginId() returns the id as a DataType rather than the id defined at the top of the class)
Did a test:

$node = entity_load('node', 1);
// TextItem
$body = $node->get('body');
$field_item = $body->offsetGet(0);
$plugin_id = $field_item->getPluginId();
dsm($plugin_id); // Output => field_item:text_with_summary
// IntegerItem
$nid = $node->get('nid');
$nid_0 = $nid->offsetGet(0);
$plugin_id = $nid_0->getPluginId();
dsm($plugin_id); // Output => integer_field

Looks like FieldItem::getPluginId() has returned "id defined at the top of the class" when it comes to configurable fields. As for base fields atm, it should return id as DataType?

I guess duplicate caches comes from a field_type plugin cached/managed by two independent managers, i.e. FieldTypePluginManager & TypedDataManager at the same time. How about we make FieldTypePluginManager extends TypedDataManager, insead of DefaultPluginManager. So the two plugin managers could share the same cache backend?

dsm($plugin_id); // Output => field_item:text_with_summary
That's what I'm talking about, 'field_item:text_with_summary' instead of 'text_with_summary'. The other example for integer_field is because "base field types" are currently not exposed as FieldType plugins, which exactly what this patch is about.

StatusFileSize
new792 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

@yched, yeah, right, I see your point.

I think concerns at #12 deserves its own issue...
Anyway, two ideas here:

1). FieldTypePluginManager extends TypedDataManager to share the same cacheback. When FieldTypePluginManager needs to get plugin definition(s), it could filter the defintions(start with 'field_item') returned by TypedDataManager. This probably solves the duplicate cache issue, but TypedDataManager is still responsible for instantiate field items.

2). Override ItemList's createItem method for configfield. Here's a little patch to show this. It will definitely fail:)
Thing is TypedDataManager is probably also responsible for instantiating ConfigField too, or even entity itself?

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, instantiate_FieldItem_via_FieldTypePluginManager.patch, failed testing.

FieldTypePluginManager extends TypedDataManager would still lead to duplicate static cache of plugin definitions. The moment two separate manager services are exposed, two separate objects are created, each with its own 'cache' property.

Note regarding the current patch:
A "configurable" field type already has to implement ConfigFieldItemInterface. So rather than requiring an additional explicit 'configurable' = TRUE property in the annotation, couldn't we automatically derive this from whether the class implements ConfigFieldItemInterface ?

Status:Needs work» Needs review
StatusFileSize
new16.38 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-roll, no other changes yet.

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.5 KB
new11.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,898 pass(es).
[ View ]

Removed a weird merge conflict.

StatusFileSize
new23.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,762 pass(es), 149 fail(s), and 54 exception(s).
[ View ]

Ok, major update. Moving all field types over and adding bc layer to baseFieldDefinitions() that translates the type names, so that we don't have to change them all.

This seems to be working pretty fine and is an important preparation for using the field type plugin manager for field items.

One major problem is that we now have conflicts, e.g. entity_reference is defined twice, and we have things like date/datetime. Some could be merged (like email, which is weird anyway) but we need to rename one of the entity references...

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-23.patch, failed testing.

Status:Needs work» Postponed
StatusFileSize
new5.69 KB
new25.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type-2023563-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

As an example, started to merge the emailitem classes together but failing due to the missing FieldDefinition, so postponed on #1994140: Unify entity field access and Field API access.

Status:Postponed» Needs review
StatusFileSize
new1.04 KB
new27.15 KB
FAILED: [[SimpleTest]]: [MySQL] 54,186 pass(es), 570 fail(s), and 263 exception(s).
[ View ]

Falling back for base fields implemented, email field tests passed fine. Others are going to be tougher :)

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
new30.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,630 pass(es), 182 fail(s), and 56 exception(s).
[ View ]

Fixing comment related tests.

Main problem at this point is all the entity reference classes, need to make sense of that stuff somehow.

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-28.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.19 KB
new30.34 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fixing various tests, CommentItem shouldn't contain entity_reference_field subproperties, attempt to replace the used class for entity reference fields instead of defining separately. Seems to be working surprisingly well (just one weird test fail in the UI from what I can see). We'll be able to clean the class hierarchies up there later on and might be able to move more into the base class (I'd like to avoid another base class in between but we need to convert taxonomy first).

StatusFileSize
new7.22 KB
new36.92 KB
FAILED: [[SimpleTest]]: [MySQL] 58,660 pass(es), 105 fail(s), and 33 exception(s).
[ View ]

Wrong patch.

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.05 KB
new36.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,484 pass(es), 23 fail(s), and 6 exception(s).
[ View ]

Killed custom list_class definitions. This should hopefully fix those fails.

Status:Needs review» Needs work
Issue tags:-Entity Field API

The last submitted patch, d8_entity_field_type-2023563-33.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, d8_entity_field_type-2023563-33.patch, failed testing.

Priority:Normal» Major
Status:Needs work» Needs review
Issue tags:+sprint
StatusFileSize
new3.04 KB
new39.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,309 pass(es), 20 fail(s), and 1 exception(s).
[ View ]

Interesting. Fixed the notices in field_help(), and the altering of the entity reference field item caused the auto-create/save to be triggered for uid 0. Added two separate fixes there, both should fix it but I think both makes sense on their own.

This is the kind of surprises we could get with this approach. Thoughts about it? I think it's probably the most controversal part about this issue.

The main problem is that the behavior of entity reference fields change based on whether entity_reference.module is enabled or not. Conceptually, that's not too different from what e.g. date.module does (altering the node create form).

Talked with @amateescu about the configurable property on in the @FieldType definition, this is not a problem as base field definitions are accessed through FieldDefinition, which has a hardcoded return FALSE in isFieldConfigurable(). The flag on that level is just that it's possible to create configurable fields with that type, e.g. it will show up in the UI.

Adding sprint tag and raising to major, this is necessary to continue with our typed data plans (Create field item classes through the field type plugin manager)

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-37.patch, failed testing.

Small review of the code:

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.php
    @@ -52,6 +52,22 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
    +  }
    +
    +
    +  /**

    Extra empty line.

  2. +++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.php
    @@ -81,4 +97,27 @@ public function getDefaultInstanceSettings($type) {
    +   * @param Boolean $configurable
    +   *   Configurable flag to indicate whether return configurable field types.
    ...
    +  public function getDefinitions($configurable = NULL) {

    Boolean -> bool. The function paramater should default to FALSE instead of NULL.
    And how about this text: "(optional) Whether to return only configurable field types. Defaults to FALSE."

  3. +++ b/core/lib/Drupal/Core/Entity/Plugin/field/field_type/BooleanItem.php
    @@ -2,23 +2,21 @@
    + *   configurable = false

    Shouldn't this be uppercase? Same for all occurences..

  4. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -51,11 +51,8 @@ public function getPropertyDefinitions() {
    -          'type' => 'entity_reference_field',
               'label' => t('Last comment ID'),
    -          'settings' => array(
    -            'target_type' => 'comment',
    -          ),
    +          'type' => 'integer',

    Is this necessary because comment.module does not depend on entity_reference? Also, 'type' should be kept above 'label'.

  5. +++ b/core/modules/entity_reference/entity_reference.module
    @@ -15,11 +15,16 @@
    +  // Make he entity reference field configurable.

    "the" :)

About the entity reference field, I think I'm starting to like this approach.. at least it's better than having two different field type names.

What I'm still a bit worried about is: what does this mean for taxonomy/file/image reference field types which also extend the base entity reference field type (through ConfigEntityReferenceItemBase)? I think the answer is "nothing changes", but I'd still like a confirmation :)

Great work!

+++ b/core/modules/user/lib/Drupal/user/Entity/User.php
@@ -58,6 +58,14 @@ public function id() {
   /**
    * {@inheritdoc}
    */
+  public function isNew() {
+    // ID 0 does not imply a new user.
+    return !empty($this->enforceIsNew) || $this->id() === NULL;

Should we do it that way even in the generic implementation? (Maybe as follow-up). The ER-change kind of makes the assumption already that only a NULL id means the id is unset.

+++ b/core/modules/field/field.module
@@ -127,17 +127,21 @@ function field_help($path, $arg) {
+        // @todo: Throws notices because the owner of field types in
+        //   \Drupal\Core\Entity is 'Core'.

Maybe the comment should just say that core components may be in there and are just skipped as they do not have help.

+++ b/core/modules/field/field.module
@@ -127,17 +127,21 @@ function field_help($path, $arg) {
+        // @todo: Throws notices because the owner of field types in
+        //   \Drupal\Core\Entity is 'Core'.

Maybe the comment should just say that core components may be in there and are just skipped as they do not have help.

+++ b/core/lib/Drupal/Core/Entity/Annotation/FieldType.php
@@ -112,10 +111,16 @@ class FieldType extends Plugin {
-   * If TRUE, fields of this type can only be created programmatically.
+   * If TRUE or the field type is not configurable, fields of this type can only
+   * be created programmatically.

Maybe say cannot be created via the UI, as configurable fields could be created via config also which does not necessarily map to "programmatically".

+++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.php
@@ -81,4 +97,27 @@ public function getDefaultInstanceSettings($type) {
+   * @param Boolean $configurable
+   *   Configurable flag to indicate whether return configurable field types.

bool|null instead of Boolean? Also, it should mentioned default behavior.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
@@ -68,11 +65,8 @@ public function getPropertyDefinitions() {
         'last_comment_uid' => array(
-          'type' => 'entity_reference_field',
+          'type' => 'integer',

Great to see that working, but could we stay with the old data types in the array for now?

This would simplify the work of the BC-layer in the class-based definitions patch as it would have to deal with only one case.

+++ b/core/modules/user/lib/Drupal/user/Entity/User.php
@@ -58,6 +58,14 @@ public function id() {
   /**
    * {@inheritdoc}
    */
+  public function isNew() {
+    // ID 0 does not imply a new user.
+    return !empty($this->enforceIsNew) || $this->id() === NULL;

Should we do it that way even in the generic implementation? (Maybe as follow-up). The ER-change kind of makes the assumption already that only a NULL id means the id is unset.

Status:Needs work» Needs review
StatusFileSize
new8.89 KB
new41.94 KB
FAILED: [[SimpleTest]]: [MySQL] 57,086 pass(es), 20 fail(s), and 37 exception(s).
[ View ]

Thanks for the reviews!

@amatescu:
1. Fixed.
2. Fixed.
3. As discussed, CommentItem is a field item, it shouldn't have field item properties. I have no idea why this works :) Let's create a field item with lists of entities :p
4. Fixed.

Yes, for now, nothing changes. I'd like to reduce the hierarchy level that we have there, but not here and not until all of them are converted to @FieldType plugins.

@fago:

Update Dreditor! ;) Also looks like various points of your review are duplicated.

1. Moved it up, let's see if this breaks something.
2. Updated comment.
3. Not touching the configurable documentation. Right below is no_ui, which is specifically a configurable field that does not have a UI. We might unify them later on, but not now.
4. Yep, went with @amateescu's suggestion.
5. See the first 3.

Also fixed the tests. EntityFieldTest was trivial, had to go back to check !$target_id instead !== NULL as apparently target_id is 0 on auto-create, not sure who sets it. Anyway, in combination with the isNew() change, this should be fine. UserValidationTest is interested, the e-mail validation is just fine, the only problem is that we get two violations now in case of an invalid e-mail. It passes with the change I made, not sure if we can go on like this, I know there is an issue to fix that weirdness with translatable/dynamic violation strings..

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-41.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
new42.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,380 pass(es).
[ View ]

Interesting, so making that the default didn't work. I'd suggest to consider that in a separate issue.

Update Dreditor! ;)

Finally, done ;-)

Interesting, so making that the default didn't work. I'd suggest to consider that in a separate issue.

Weird, but yeah - sounds good.

3. Not touching the configurable documentation. Right below is no_ui, which is specifically a configurable field that does not have a UI. We might unify them later on, but not now.

I was not suggesting to touch configurable documentation, but I think that this change is not necessarily true:

+++ b/core/lib/Drupal/Core/Entity/Annotation/FieldType.php
@@ -112,10 +111,16 @@ class FieldType extends Plugin {
-   * If TRUE, fields of this type can only be created programmatically.
+   * If TRUE or the field type is not configurable, fields of this type can only
+   * be created programmatically.
    *
    * @var boolean
    */

field types that are configurable, can not only be created programmatically but via config also. That's why I suggested to go with "cannot be created via the UI" instead of "can only be created programmatically.

I know what you meant, but "cannot be created via the UI" is exactly how the no_ui flag is documented below. And they're not the same. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

This is really impressive work!

Here's part review. Will check left later..

<?php
use Drupal\Component\Annotation\Plugin;
+use
Drupal\Core\TypedData\Annotation\DataType;
?>

Let's remove the redundant one, i.e, Annotation\Plugin:)

<?php
+  public function getDefinitions($configurable = FALSE) {
+   
$definitions = $this->getCachedDefinitions();
+    if (!isset(
$definitions)) {
+     
$definitions = $this->findDefinitions();
+     
$this->setCachedDefinitions($definitions);
+    }
+    if (
$configurable) {
+      return
array_filter($this->definitions, function ($definition) {
+        return
$definition['configurable'];
+      });
+    }
+    return
$definitions;
+  }
?>

Maybe something like this to save code?

<?php
+  public function getDefinitions($configurable = FALSE) {
+   
$definitions = parent::getDefinitions();
+    if (
$configurable) {
+      return
array_filter($this->definitions, function ($definition) {
+        return
$definition['configurable'];
+      });
+    }
+    return
$definitions;
+  }
?>

<?php
+ * @FieldType(
+ *  
id = "boolean",
+ *  
label = @Translation("Boolean"),
  *  
description = @Translation("An entity field containing a boolean value."),
- *  
list_class = "\Drupal\Core\Entity\Field\FieldItemList"
+ *   configurable = false
- * @DataType(
- *  
id = "date_field",
- *  
label = @Translation("Date field item"),
+ * @
FieldType(
+ *  
id = "date",
+ *  
label = @Translation("Date"),
  *  
description = @Translation("An entity field containing a date value."),
- *  
list_class = "\Drupal\Core\Entity\Field\FieldItemList"
+ *   configurable = false
 
* )
?>

Maybe configurable = FALSE, and for other field types?

<?php
-use Drupal\Core\TypedData\Annotation\DataType;
-use
Drupal\Core\Annotation\Translation;
use
Drupal\Core\Entity\Field\FieldItemBase;
?>

Translation has been deleted for all new converted Basic field_types? Maye we still need them?
And we don't need Drupal\Core\Entity\Annotation\FieldType?

<?php
- * @DataType(
- *  
id = "boolean_field",
- *  
label = @Translation("Boolean field item"),
+ * @
FieldType(
+ *  
id = "boolean",
+ *  
label = @Translation("Boolean"),
  public function
getPropertyDefinitions() {
    if (!isset(static::
$propertyDefinitions)) {
      static::
$propertyDefinitions['value'] = array(
       
'type' => 'boolean',
       
'label' => t('Boolean value'),
      );
    }
    return static::
$propertyDefinitions;
  }
?>

During the converting process, some field types (boolean, email, float, integer, language, string, uri, uuid) get new id without '_field' suffix. Will that confuse the $propertyDefinitions's property type?
Like this 'boolean' field type, both its id and property's type are called 'boolean'. I sense property's type means DataType, instead of FieldType. But it might be better to reserve the suffix '_field' suffix for FieldType to distinguish it from DataType imo.

<?php
/**
  * @file
- * Contains \Drupal\email\Plugin\field\field_type\ConfigurableEmailItem.
+ * Contains \Drupal\Core\Entity\Plugin\field\field_type\EmailItem.
  */
-namespace Drupal\email\Plugin\field\field_type;
+namespace
Drupal\Core\Entity\Plugin\field\field_type;
-use
Drupal\Core\Entity\Annotation\FieldType;
-use
Drupal\Core\Annotation\Translation;
-use
Drupal\Core\Entity\Plugin\DataType\EmailItem;
use
Drupal\field\FieldInterface;
+use
Drupal\field\Plugin\Type\FieldType\ConfigFieldItemBase;
/**
- * Plugin implementation of the 'email' field type.
+ * Defines the 'email' entity field type.
  *
  * @FieldType(
  *   id = "email",
?>

This doesn't look good:( Core will depend on field module. It's not a good infrastructure to me:) kind of regression.
I'd suggest to keep this email field at email module. Also, since it's configurable, I think we need to keep the old doc(and default widget), insead of " the 'email' entity field type.".

<?php
        
'cid' => array(
-         
'type' => 'entity_reference_field',
+         
'type' => 'integer',
          
'label' => t('Last comment ID'),
-         
'settings' => array(
-           
'target_type' => 'comment',
-          ),
         ),
?>

Any particular reason to not use entity reference field here?

StatusFileSize
new7.14 KB
new42.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]

Thanks for the review. As I told @fago, numbered bullet point help to reference to your review, dreditor does that automatically now.

- Removed use.
- Simplified getDefinitions()
- Changed to uppercase FALSE, already discussed that above but forgot to apply.
- Annotation classes no longer need to be use'd, they just work.
- The BooleanItem has field type "boolean" and data type "field_item:boolean", Boolean has just data type "boolean". "field_item:boolean_field" would be repetitive, as would field type "boolean_field". That's like saying node is "node_entity". Also note that this is an intermediate step, #2047229: Make use of classes for entity field and data definitions will result in typed objects for the field definitions, and they will have a getType (or DataType()) method and a getFieldType().
- field is a required module. All of them are defined as a field/field_type plugin (that is defined core but uses the field namespace). That said, this, too, is a intermediate step. We in the process of moving the field plugin types to Core\Entity, together with all interfaces that are relevant. I think this is fine. email.module currently continues to provide the widget and formatter and does add that in an alter hook. As with entity_reference, we could add the configurable flag there, not sure. We should probably decide on a case by case base which modules still makes sense, I'd be happy to let email go but many others, more complicated ones might still make sense. We'll see.
- Everyone points that out, see #41.3 (the first).

Oh, I understand why entity_reference_field worked. Because it's never used ;) Nothing ever did $node->comment_field->get('cid'), so it never even looked at the type but relied on the lazy-loading things to just write and get it from $this->values. It could be type = 'banana' and would work just as well ;)

1).

The BooleanItem has field type "boolean" and data type "field_item:boolean", Boolean has just data type "boolean".

Thanks for clarifying that:) But does that still result in duplicate data cache? TypedDataManager and FieldTypeManager will still cache one field_item in the same time? I thought we are solving this problem in this issue?

2).
I'm not sure how $configurable and $no_ui work together.

<?php
  
/**
    * A boolean stating that fields of this type are configurable.
    *
-   * @todo: Make field module respect this.
-   *
    * @var boolean
    */
  
public $configurable = TRUE;
@@ -
112,10 +110,16 @@ class FieldType extends Plugin {
  
/**
    * A boolean stating that fields of this type cannot be created through the UI.
    *
-   * If TRUE, fields of this type can only be created programmatically.
+   * If TRUE or the field type is not configurable, fields of this type can only
+   * be created programmatically.
    *
    * @var boolean
    */
  
public $no_ui = FALSE;
?>

I think we only need one? Maybe we still don't reach the consensus that a configurable field should be available at UI, while a non-configurable field should not. But the problem is we get all field types, including those base field types, available at field ui now. We have two date, integer, boolean, float fields etc, at field ui:)
Update:
And Email field type, even email module is not enabled, and I just tried to add configurable = FALSE to EmailItem annotation, it still shows in the field ui. In fact, it should not, becaue it doesn't have formatter and widget prepared for it.

no_ui was added in D7 for the case of a contrib module willing to include field types with some specific business logic, for which "add new ones to your heart's content" is not wanted, but benefitting on the flexibility of field API (configurable widgets & formatters). It does look like it somewhat duplicates "not configurable". I.e, those use cases would probably define "non configurable" field types now, and define the fields as base fields ?

More thinking..
1).

<?php
-      $this->instance = $instances[$this->getName()];
+      if (isset(
$instances[$this->getName()])) {
+       
$this->instance = $instances[$this->getName()];
+      }
+      else {
+       
// For base fields, fall back to the parent implementation.
+        // @todo: Inject the field definition with
+        //   https://drupal.org/node/2047229.
+        return parent::getFieldDefinition();
+      }
?>

I feel it's not necessary here. ConfigFieldItemList should only contain config FieldItem, not base field items. If $instances[$this->getName()] may doesn't exist, then I will start to think maybe something went wrong:)

2).

<?php
// Make the entity reference field configurable.
$info['entity_reference']['configurable'] = TRUE;
$info['entity_reference']['class'] = '\Drupal\entity_reference\Plugin\field\field_type\ConfigurableEntityReferenceItem';
$info['entity_reference']['list_class'] = '\Drupal\field\Plugin\Type\FieldType\ConfigFieldItemList';
$info['entity_reference']['settings']['target_type'] = \Drupal::moduleHandler()->moduleExists('node') ? 'node' : 'user';
$info['entity_reference']['instance_settings']['handler'] = 'default';
$info['entity_reference']['instance_settings']['handler_settings'] = array();
$info['entity_reference']['default_widget'] = 'entity_reference_autocomplete';
$info['entity_reference']['default_formatter'] = 'entity_reference_label';
$info['entity_reference']['constraints'] = array('ValidReference' => TRUE);
?>

Some lines might be avoided.
<?php
$info
['entity_reference']['list_class'] = '\Drupal\field\Plugin\Type\FieldType\ConfigFieldItemList'; // This will be added at EntityManager::processDefinition().
$info['entity_reference']['constraints'] = array('ValidReference' => TRUE); // EntityReferenceItem has it covered in its annotation.
?>

3). Regarding #2), any benifits of implementing it this way, i.e., use $info['entity_reference']['class'], instead of exposing ConfigurableEntityReferenceItem to FieldType directly? I thought one new independent FieldType might be more clearer?

4).

Oh, I understand why entity_reference_field worked. Because it's never used ;) Nothing ever did $node->comment_field->get('cid'), so it never even looked at the type but relied on the lazy-loading things to just write and get it from $this->values. It could be type = 'banana' and would work just as well ;)

I found this rule applies everywhere:) See Node::baseFieldDefinitions. Those properties types remains the old way in this patch, but they work well! Of course, something might be broken, but we didn't notice it from Tests inside Drual HEAD now.
Anyway, some have been updated, like

<?php
  
if ($entity_type === 'taxonomy_term' || $entity_type === 'node') {
    
$info['definitions']['path'] = array(
-     
'type' => 'path_field',
+     
'type' => 'field_item:path',
?>

Assume they actually work(because currently, they don't work, i.e., type could be anything^^), I feel we should use path directly, instead of field_item:path. One benifit of this issue I could see, is trying to distinguish FieldType from DataType. When we need to do, maybe getPropertyInstance for Entity(this function is used at ContentEntityBase::getTranslatedField()), e.g., node, user, we could use something like, FieldTypeManager::createInstance() or FieldTypeManager::create() to instantiate these properties, instead of \Drupal::typedData()->getPropertyInstance().

- Yes, this still results in duplicated caches, that will not change. What we will change is which plugin manager instantiates field item objects. that will change but not here. But we need to change this so that all field item classes are field type plugins to make that possible in a next step.

- Yes, the fact that base fields show up there is simply a bug. We extend the method to allow to return only configurable field types but we don't use that yet. We obviously need to, then they will go away. I'd prefer a getConfigurableDefinitions() or something instead of an optional argument, though. But that's an easy change, will change that on the next re-roll.

- No, no_ui and configurable are *not* the same. no_ui can still exist as configurable fields with a field config and field instance config to back them, create storage and so on. Yes, we might at some point be able to have pluggable storage for non-configurable fields too, but we don't have that now. So we can't unify them *yet*.

#51
1. It is necessary, the comment explains why. Once something can have configurable fields but is also used for base fields, we need to support both. As the @todo explains there, this is another temporary thing until we can inject them the right definition object from the plugin manager. Then they won't have to worry about it and will just do a return $this->definition.

2. This happens *after* processDefinition(), we need to specify the class. I tried ;) The constraint is duplicated, true.

3. Naming, then they'd need a separate name. entity_reference_base for the base one? entity_reference_configurable for the configurable? The first seems weird when you have to define it as a base field, the other one would mean that we have to change all code that references it somehow. That's why I went with extending it, but as mentioned above, this is the most controversial part but it seems like the best option to me.

4. No, they are very much not the same. The type of base fields is important, but there's a simple preg_replace() in EntityManager that converts *_field to field_item:*. Converting them here would make the patch twice as big and they will change anyway again in #2047229: Make use of classes for entity field and data definitions (to setFieldType('something') or so), so this is much easier. However, that BC layer only applies to base fields, that's why path has to change. And yes, path will have to change again (in the mentioned issue or the one that will switch to use the field type plugin manager, or both). And then it will change to just 'path'. So, yes, we came to the same conclusion as you in Prague, but we need to do it step be step.

Status:Needs review» Needs work

Marking needs work for the configurable thing, will work on that later today.

StatusFileSize
new7.96 KB

I'd prefer a getConfigurableDefinitions() or something instead of an optional argument, though

Looks like adding no_ui = TRUE works. When one class needs to serve both base field and configurable field, no_ui need to be updated too. See interdiff. Also, find a code stye thing

<?php
+ $info['email']['default_widget'] = 'email_default';
   if (\
Drupal::moduleHandler()->moduleExists('text')) {
?>

Need one more space for new line:)

3. Naming, then they'd need a separate name

I feel current name works fine, i.e., EntityReferenceItem and configurableEntityReferenceItem. But whatever, current code works well too:)

Status:Needs work» Needs review
StatusFileSize
new10.46 KB
new48.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,781 pass(es).
[ View ]
new45.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,454 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Instead of adding it manually, we could also add it in processDefinition() if configurable == FALSE. But I think we should just be specific about which kind of fields we want in the field UI's. Attached patch changes most calls that make sense to getConfigurableDefinitions().

Also made the email field item not configurable by default as we don't have a widget/formatter, similar to entity_reference.

About the naming, it's not the class name that's the problem, it's the plugin id. Right now, we have entity_reference_field for the base field and field_item:entity_reference for the other one, now they're both the same, just like e-mail. So we would have to rename one of them.

Also added some test coverage for this, the test patch should fail now. While writing these tests, I also noticed that the existing check for the no_ui flag there was broken as the selector didn't match the select anymore. The usual problem with negative assertions. It also has the same problem as #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0, so fixed that too.

Patch looks good to me. Some questions/remarks:

- Which email constraints get how doubled?
- ER fields as part of a comment item - sounds a bit like field collection to me ;-)
- Still not sure about the UI flag, let's try to discuss on IRC

StatusFileSize
new1.55 KB
new48.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,787 pass(es).
[ View ]

Ok, no_ui thing was a communication problem. We agreed on removing the second sentence, nothing useful/correct in there.

Added the link to an issue that should help with the @todo in the user validation test. The only problem there is that email item wants to have a better error message but itself only validates a subset of being a valid e-mail.

Thanks, this should be good to go then.

Good work! getConfigurableDefinitions() is a good solution to handle the no_ui stuff.

1. re#52

It is necessary, the comment explains why. Once something can have configurable fields but is also used for base fields, we need to support both.

Regarding this, I still feel it's a bit wierd. No matter an item class is used by both base field and configurable field or not, ConfigFieldItemList should ALWAYS contain a list of configurable item class(even this item might be used for base field too, but it should still be configurable, otherwise we get big problems here I think.)

The scenario as you mentioned perhaps happens the other way round, i.e, for FieldItemList, it needs to support configurable
item class too. Or if there's a smiliar situation for FieldItemList as ConfigFieldItemList here, it's worthy to add support for configurable item class, because those "base field item class" could also be "configurable item class".

But in my first thinking, they should work independently in specific context, i.e., when it comes to ConfigFieldItemList, we only need to treat the item class as configurable item class; when it comes to FieldItemList, we only need to treat the item class as "base field item class". Am I right?:)

2.

<?php
- *   description = @Translation("An entity field containing an e-mail value.")
+ *  
description = @Translation("An entity field containing an e-mail value."),
+ *  
configurable = FALSE
 
* )
  */
class
EmailItem extends ConfigFieldItemBase {
?>

Since EmailItem isn't configurable here, then we should probably dismiss ConfigFieldItemBase here. I'd suggest we
use the same trick as for entity_reference. In email_field_info_alter, add a line $info['email']['class'] = 'ConfigurableEmailItem', and then make ConfigurableEmailItem extend EmailItem implement ConfigFieldItemInterface. In this way, we still reuse the code, and only expose one 'field_item:email' to Drupal.

3.

<?php
@@ -33,7 +33,7 @@ function field_info_field_types($field_type = NULL) {
     return \
Drupal::service('plugin.manager.entity.field.field_type')->getDefinition($field_type);
   }
   else {
-    return \
Drupal::service('plugin.manager.entity.field.field_type')->getDefinitions();
+    return \
Drupal::service('plugin.manager.entity.field.field_type')->getConfigurableDefinitions();
   }
?>

While this is deprecated, I guess we still need to update doc for this function if we update code like this. On the other
hand, I'm not sure we really need this update here.

as of Drupal 8.0. Use \Drupal::service('plugin.manager.entity.field.field_type')->getDefinition() or \Drupal::service('plugin.manager.entity.field.field_type')->getDefinitions()

4.

<?php
+      else {
+       
$this->assertFalse($this->xpath('//select[@id="edit-fields-add-new-field-type"]//option[@value=:field_type]', array(':field_type' => $field_type)), String::format('Configurable field type @field_type is no available.', array('@field_type' => $field_type)));
?>

This line should probably be 'Non-configurable field type...'

otherwise, this patch looks good to go:)

1. I don't see why this is a problem. ConfigFieldItem(List) support configurable fields that are backed by a field instance. We can't have a dynamic class hierarchy, all fields that want to support that need to support both. I don't see a problem with that. And this code is temporary. They will just get a $definition object that implements FieldDefinitionInterface and is either FieldDefinition (base field) or a FieldInstance (configurable field). It is even possible that we will merge the additional methods of the config variants together into the normal ones.

2. This doesn't change anything. the replacement is permanent. As soon as entity_reference.module is enabled, *all* entity_reference fields, base field or not, use the extended version that supports configurable fields. The same would be the case for email. For entity_reference, the separation makes sense, because it has tons of features that are not relevant when used as a base field. For email, there are currently two simple methods, getConstraints() will go away when we solve the validation message problem and schema() method move up anyway. Also, that class would have to implement the method of ConfigurableFieldItemInterface itself instead of being able to rely on a base implementation.

3. Good point, I'm not sure. The direct replacement is getConfigurableDefinition() but we will probably remove this function before 8.0, so I don't think it's that important.

4. Yes, it should be. I thought I wrote that...

StatusFileSize
new1.81 KB
new49.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8_entity_field_type-2023563-61.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed 3. and 4.

Status:Needs review» Reviewed & tested by the community

Ok, since no body else argues against with 1) and 2), I'm fine with it too... Let's do this.

Issue tags:-sprint, -Entity Field API

Status:Reviewed & tested by the community» Needs work
Issue tags:+sprint, +Entity Field API

The last submitted patch, d8_entity_field_type-2023563-61.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new49.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,502 pass(es).
[ View ]

Re-roll. Tiny conflict in FieldInfoTest.

Status:Reviewed & tested by the community» Needs review

This patch looks awesome. Just some minor questions/feedback. Setting to "needs review" for #3 below. The other stuff can be follow up, unless a patch is being rerolled anyway.

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.php
    @@ -81,4 +96,17 @@ public function getDefaultInstanceSettings($type) {
    +    $definitions = parent::getDefinitions();

    Wouldn't this be more logical as $this->getDefinitions(), in case a getDefinitions() method is later added to this class?

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/field/field_type/EmailItem.php
    @@ -2,28 +2,56 @@
    +class EmailItem extends ConfigFieldItemBase {

    A comment here explaining why EmailItem extends ConfigFieldItemBase even though it's configurable = FALSE would be helpful.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -68,11 +65,8 @@ public function getPropertyDefinitions() {
             'last_comment_uid' => array(
    -          'type' => 'entity_reference_field',
    +          'type' => 'integer',
               'label' => t('Last comment user ID'),
    -          'settings' => array(
    -            'target_type' => 'user',
    -          ),

    Why?

  4. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/field_type/ConfigurableEntityReferenceItem.php
    @@ -7,33 +7,13 @@
      * Plugin implementation of the 'entity_reference' field type.
      *
    - * @FieldType(

    Would be good to add a comment here as to why this class doesn't have any annotation, despite being in the Plugin directory and being described as a "Plugin implementation ...".

  5. +++ b/core/modules/field/field.module
    @@ -99,19 +99,23 @@ function field_help($path, $arg) {
           foreach (array_merge($field_types, $field_widgets) as $field_module) {
    -        $modules[] = $field_module['provider'];
    +        $providers[] = $field_module['provider'];
           }

    Not this issue's fault, but $field_module seems like a terrible name for this local variable. Can we fix that while we're in here?

Issue summary:View changes

Updated issue summary.

re 3: Hah, you're like the fifth person to point that out. The old code was bogus, this is a field item, it can't contain other field items inside it :)

Added this to the issue summary:

- CommentItem changes: > It has field properties defined as entity_reference_field. This is completely bogus, a field item can't have field types as properties. It only works because we never instantiate it thanks to the lazy-loading logic in field items.

See #48 why it worked.

The other points are totally valid, will re-roll tomorrow with some documentation improvements.

Status:Needs review» Reviewed & tested by the community

Thanks, back to RTBC then. If you reroll for the other stuff before commit, great. But if a committer wants to commit this before then, also great.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new4.18 KB
new50.42 KB
FAILED: [[SimpleTest]]: [MySQL] 57,746 pass(es), 69 fail(s), and 27 exception(s).
[ View ]

1. Change, left-over of this being an override of getDefinitions()
2. Added a comment.
4. Yeah. Moved out of the plugin directory and updated the documentation, better like this?
5. $plugin?

Status:Needs review» Needs work

The last submitted patch, d8_entity_field_type-2023563-68.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new976 bytes
new50.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,610 pass(es).
[ View ]

This might fix the error.

Status:Needs review» Needs work

A couple of minors.

+++ b/core/lib/Drupal/Core/Entity/Plugin/field/field_type/EmailItem.php
@@ -13,6 +13,13 @@
+ * This field type extends is not configurable but extends from
+ * ConfigFieldItemBase to make it easy for email.module to add the necessary
+ * default formatter and widget and make it configurable.

Not sure that this new comment to explain why EmailItem extends ConfigFieldItemBase makes sense.

Also I'm wondering why we are using two different patterns here - one for email and one for entity_reference where we swap out the entire class to make it configurable. Compare email_field_info_alter vs entity_reference_field_info_alter

+++ b/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.php
@@ -8,6 +8,7 @@
+use Drupal\Component\Utility\NestedArray;

Unnecessary as far as I can see.

Status:Needs work» Needs review
StatusFileSize
new7.99 KB
new48.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert_entity_field_type-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, as discussed, here's an implementation that uses the same approach for email and extends the base email item class.

Somehow I thought the overhead would be bigger but we just need to implement two additional methods. This makes the user validation fix unecessary, the same problem will still happen when email.module is enabled but that's no longer visible in the test.

Off-topic: Anyone knows what the weird widget handling in email.module is supposed to do? Why does it use the text version when text.module is enabled (which it always is, so it never uses it's own?). I kept it because that's how it works right now but it makes zero sense.

Issue tags:-sprint, -Entity Field API

#73: convert_entity_field_type-73.patch queued for re-testing.

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

The last submitted patch, convert_entity_field_type-73.patch, failed testing.

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new48.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,637 pass(es).
[ View ]

Re-roll, updated issue summary. Who wants to RTBC? :)

Re #73 interdiff
1.

<?php
namespace Drupal\Core\Entity\Plugin\field\field_type;
use
Drupal\field\FieldInterface;
-use
Drupal\field\Plugin\Type\FieldType\ConfigFieldItemBase;
+use
Drupal\Core\Entity\Field\FieldItemBase;
/**
  * Defines the 'email' entity field type.
?>

FieldInterface seems unnecessary.

2.

<?php
/**
-   * Defines the max length for an email address
-   *
-   * The maximum length of an e-mail address is 254 characters. RFC 3696
-   * specifies a total length of 320 characters, but mentions that
-   * addresses longer than 256 characters are not normally useful. Erratum
-   * 1690 was then released which corrected this value to 254 characters.
-   * @see http://tools.ietf.org/html/rfc3696#section-3
-   * @see http://www.rfc-editor.org/errata_search.php?rfc=3696&eid=1690
-   */
-  const EMAIL_MAX_LENGTH = 254;
/**
-   * {@inheritdoc}
-   */
-  public function getConstraints() {
-   
$constraint_manager = \Drupal::typedData()->getValidationConstraintManager();
-   
$constraints = parent::getConstraints();
-
-   
$constraints[] = $constraint_manager->create('ComplexData', array(
-     
'value' => array(
-       
'Length' => array(
-         
'max' => static::EMAIL_MAX_LENGTH,
-         
'maxMessage' => t('%name: the e-mail address can not be longer than @max characters.', array('%name' => $this->getFieldDefinition()->getFieldLabel(), '@max' => static::EMAIL_MAX_LENGTH)),
-        )
-      ),
-    ));
-
-    return
$constraints;
-  }
?>

I feel this constraint should be kept in EmailItem. As entity field type, EmailItem also needs to validate the length constraint.

3.

<?php
/**
  * @file
- * Contains \Drupal\Core\Entity\Plugin\field\field_type\EmailItem.
+ * Contains \Drupal\email\Plugin\field\field_type\ConfigurableEmailItem.
  */
-namespace Drupal\Core\Entity\Plugin\field\field_type;
+namespace
Drupal\email;
?>

Wrong file definition:)

StatusFileSize
new976 bytes
new48.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,984 pass(es).
[ View ]

1. Fixed.
2. No, we don't. The length validation is only a small subset of validating an e-mail, we already have full e-mail validation by using type email for the ->value property. The constraint is only there because that pattern was added to all configurable fields, so that they contain the field name. The current actual patch doesn't have to touch that at all anymore, which I think is the best solution for us here.
3. Fixed.

Status:Needs review» Reviewed & tested by the community

I looked at the changes since #69.

I could see the email field type class become a single one later one, while ER-field probably won't though. Anyway, taking the same approach for both field types seems to be a reasonable thing to do at least for now.
-> Recent changes look good to me, so back to RTBC.

Title:Convert entity field types to the field_type pluginChange notice: Convert entity field types to the field_type plugin
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed 0f8b4e7 and pushed to 8.x. Thanks!

Status:Active» Needs review
Issue tags:-Needs change record

Create a record https://drupal.org/node/2111927.

As many changes in this patch are supposed to be temporary, it only specifies the difference/association between base field type and configurable field type.

I think we should extend and improve https://drupal.org/node/2064123 instead, that change notice doesn't make much sense on it's own. Will try to work on it tonight.

Title:Change notice: Convert entity field types to the field_type pluginConvert entity field types to the field_type plugin
Status:Needs review» Fixed

Ok, updated https://drupal.org/node/1806650/revisions/view/2831231/2877845 and https://drupal.org/node/2064123/revisions/view/2862015/2877871.

Also updated the change notice by @smiletrl to clarify and reference the other two change notices: https://drupal.org/node/2111927/revisions/view/2876689/2877885

There will be at least two additional issues (changing baseFieldDefinitions and field definition structures to classes) and using the field type plugin manager to create field item classes, I'll continue to update those change notices as we go on.

Closing, thanks for all the reviews, see you all in #2047229: Make use of classes for entity field and data definitions!

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

Issue summary:View changes

Updated issue summary

Issue tags:-sprint