Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList

Still +1 on just Field ;)

Drupal\Core\Entity\Field\FieldDefinition (current name in the patch) --> Field ? BaseField ?

I'd be ok with just Field, looks cleaner and does not introduce cognitive burden at first sight ;)

So this issue is about acknowledging that "Field" is about field definitions, and deriving a clear and sound naming scheme from there - hopefully once and for all...

Great!

Drupal\Core\Entity\Field\FieldDefinitionInterface --> FieldInterface

Agreed. That's an obvious consequence of above.

Drupal\field\Plugin\Type\FieldType\ConfigFieldItemBase -> ConfigurableFieldItemBase
Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface -> ConfigurableFieldItemInterface

I like it, since everywhere else in core that we use the "config" abbreviation, it stands for "configuration", never (AFAIK) for "configurable". And here, we're not dealing with a "field item configuration", or a "configuration item", but rather, with a field item that happens to be configurable (it provides settings forms). We also identified this before API freeze (#1969728-86: Implement Field API "field types" as TypedData Plugins), but I don't know if these are sufficient arguments for making the change now that we're past API freeze, since this isn't needed to achieve the issue's primary scope of clarifying "field" vs. "field definition".

Drupal\field\FieldInterface --> ConfigurableFieldInterface
Drupal\field\FieldInstanceInterface --> ConfigurableFieldInstanceInterface
Drupal\field\Plugin\Core\Entity\Field --> ConfigurableField
Drupal\field\Plugin\Core\Entity\FieldInstance --> ConfigurableFieldInstance

Is this a sufficient improvement to be worth a post-API-freeze API change? Presumably, the reason for this is to disambiguate Drupal\field\FieldInterface from Drupal\Core\Entity\Field\FieldInterface, but do we really need that disambiguation? It would be really good to get feedback from people who are likely to use these classes/interfaces (i.e., people altering Field UI forms, building alternate field configuration UIs, or reacting to CRUD hooks when field/instance configurations are changed). I do like the consistency this would introduce with ConfigurableFieldItem* above.

Meanwhile, shameless plug for #2058257: Fix ConfigFieldItemInterface's method signatures to typehint interfaces instead of classes.

It really feels like we need to sort this out, so tagging accordingly. However, we should really timebox this discussion, because it's going to create a hot mess when it's done.

How about whatever the best proposal is by August 21 (two weeks from now), let's go with it. I'd love to commit a patch for this by the end of the month.

My drive-by review is most of this looks good, but "FieldItemList" is problematic.

One thing that bugs me of the current proposal is that it seems we are using the configurable terminology as a synonym of "field defined by the Field API module". However theoretically any module can define a configurable field, it's not clear to me whether these receive any special treatement from the Entity Field API atm, but in this light I am wondering whether it makes sense to confine ConfigFieldInterface to the Field API namespace. I guess it would be perfectly legit to add a ConfigFieldInterface in the Drupal\Core\Entity namespace should we need it in the future, but still this feels a bit wrong to me.

Edit: to clarify, I am not proposing (again) to move those interfaces in the core namespace, rather I am wondering whether we can come up with a name more focused on the CMI aspect. Something like:

Drupal\field\FieldConfigInterface
Drupal\field\FieldInstanceConfigInterface
Drupal\field\FieldConfig
Drupal\field\FieldInstanceConfig

I never heard that we intended to support a 3rd kind ?

Well, the problem is that the Entity Field API defines a 'configurable' key that either is a special case for the Field module, which should not exist as the EFA should know nothing about the Field module, or a key that other modules than it can use. In the latter case how would you know what a ConfigurableFieldInterface type hint refers to, without looking at the related use statement?

I'm not convinced on the Field/FieldDefinition argument. I don't see that FieldDefinition disrupts our language much.

With entities, if I instantiate one, I have an instance of EntityInterface. Making new entities is done by creating new classes that implement that interface. Creating new fields on an entity can't be done by creating new classes, so have this concept of the field definition. I think that's a worthwhile distinction. It's one that we make in code because we have to deal with this idea of user configurable properties on an object. The term FieldDefinition is very meaningful to us, but it is necessary because of an implementation detail; It shouldn't be exposed to users or become a part of the user's language.

If I have an instance of EntityInterface, and I call $entity->getBody(), I would expect it to return an instance of FieldInterface. FieldItemListInterface would make me feel like running far far away.

Holy batman. We surely added a few references to those fields since this issue (or #1869574: Support single valued Entity fields) started ;)

I completely agree with the suggested naming here. And I'm pretty sure that fago does too, because that's where he was going in that issue above and the list class that we use here is coming from Typed Data and there, the default list class is named ItemList. Ta-da ;) (Actually, the main reason it is not named List is that this is a reserved keyword in PHP but sshh)

If you look at what we have right now:

- We have Entity Field and FieldItem
- We have field Field and FieldInstance
- Those two implement FieldDefinitionInterface
- *aaand* there is ConfigField, that extends the Entity Field for configurable fields.

Which means we have Field that defines a configurable field and ConfigField that is the field item container for configurabe fields o.0

What this patch does is add a "ItemList" to all those Entity Field classes, which gives us:
- ItemList (Already there)
- FieldItemListInterface
- FieldItemList extends ItemList
- ConfigFieldItemListInterface (yeah, a bit long)
- ConfigFieldItemList extends FieldItemList
- LegacyConfigFieldItemList (just temporary BC stuff)
- FileFieldItemList extends ConfigFieldItemList
- ImageFieldItemList extends ConfigFieldItemList

See:

Also, that patch would have been hell to create without PHPStorm. Let's see how good it is.

Wondering where the Approved API Change tag here is exactly coming from... :)

Wondering where the Approved API Change tag here is exactly coming from

#7. Not sure why it's not visible there, but that's where it happened.

This should fix those test failures.

Hm, not sure yet, haven't really read that part before :)

Wondering if we want to this rename as a separate step from the other stuff and then re-visit this.

Testbot doesn't seem to like my patch, not sure why when the previous one worked fine.

Sorry, this dropped way off my radar. And it's been so long I don't remember what my specific concerns were about FieldItemList 3 weeks ago. I'm flogging myself as we speak for not commenting in more detail at the time. :\

However, I revisited this issue today and I still don't like "Item." To try and articulate why, if you read the bold elements of the issue summary right now, you get a "plain English" way to explain what you are trying to get at with the API. And to a large extent, you are changing the APIs to match these plain English descriptions, which is good and lovely. For example, ""field definition" class for "base fields"" (plain English) == FieldBase (code).

But notice that you don't describe things you are proposing to change to "FieldItem*" as "Field items." You describe them as "Field values." So therefore, it seems like that's the more natural English way of describing them, and so that's the word that should be used.

It would also help communicate the fact that these values all end up in a column in a database somewhere. You never call these "database items." You do call them "database values," however.

This issue doesn't introduce the concept of field items. They were always called field items. (field_get_items()). We already have and continue to have FieldItem, I don't think anyone is arguing about that so far.

All my patch does is rename Field, which is quite vague and we have multiple things that are "Field" to FieldItemList, aka a list of FieldItem's. Which describes exactly what it is.

Field values is IMHO actually quite unclear. Considering $node->body[0]->value, what is the field value(s) here? body? body[0]? ->value?

On a call with webchick, Dries, and xjm, we were brainstorming different options, and the one that they all liked the most was:

Drupal\Core\Entity\Field\Field -> FieldValueList
FieldItem -> FieldValue

Considering $node->body[0]->value, what is the field value(s) here?

$node->body is "the value list". $node->body[0] is "a value" (note: a, not the). $node->body[0]->value is the value of the "value" property. Is that confusing? It's not a problem for field types that use other property names (e.g., ER (target_id) and Link (title, url)), but a lot of fields do end up using "value" as either their only or primary property.

Additionally, we currently name the method that turns the PHP object into an array as getValue(). I.e., $node->body[0] is an object; $node->body[0]->getValue() is an array representation of that object. Does that add further confusion to calling the object a "value" object? Maybe, though it is actually what it is.

This issue doesn't introduce the concept of field items. They were always called field items. We already have and continue to have FieldItem, I don't think anyone is arguing about that so far.

I think the usage of the term "field item" was less problematic in D7, when it didn't correspond to classes and interfaces. Meanwhile, I think lots of people have had an initial (negative) gut reaction to FieldItemList as a name.

I don't know how to best proceed here; we need to come to some decision fast, but whatever we pick, we'll be stuck with for a while, and fields are one of the key things that defines Drupal, so would be nice to pick something that clicks naturally for people.

Also, following the logic in #20, the very beginning of the issue summary says:

There has been a lot of uncertainty (understatement) about terminology in EntityNG / Field API, mostly because the word/class "Field" is being used:
- in EntityNG, to point to the objects holding values on a specific entity ($node_1->body)
- in "legacy" Field API, to point to definition structures ("the 'body' field on article nodes)

Again, the natural way the problem of this issue was described as was we need to distinguish value objects from definition objects, not we need to distinguish item objects from definition objects.

Uh, sorry, but that seems like a pretty crazy idea to me. Doing some searches, the concept of field items is all over the place, renaming that would be *insanely* huge.

I did a quick search on FieldItem, and we're talking about 200 different hits, not only classes but also methods like loadFieldItems in the storage controller, then 36 SomethingItem class and a lot of them have corresponding SomethingItemTest classes and, not to forget, many hundreds cases of $items, which are passed to formatters, widgets, and are in many many places outside of that as well.

And I don't get the reason for this at all, a field item is a very basic concept of Field API and IMHO well understood.

Looking at formatters/widgets for example, we right now have "FieldInterface $items", which is a bit weird but "FieldItemListInterface $items" (which is what this patch does) looks actually quite nice to me. "FieldValueListInterface $values", "$values[0]->getValue()", "A list of values that consist of values", really? :) And I thought ->value vs. getValue() is bad and ambiguous, but seems harmless compared to this.

I hope my comment doesn't sound aggressive or anything, I'm just very confused to see you argue like that..

I hope my comment doesn't sound aggressive or anything

Not at all. You raise legitimate concerns.

I'll put one other possibility on the table:
Drupal\Core\Entity\Field\Field -> FieldValue
FieldItem* -> no change

Some benefits of this:

• Not renaming all the field type classes (which are currently named TextItem, LinkItem, EmailItem, etc., and extend FieldItemBase), which would be extra core work and contrib disruption.
• Outside of the names of field type classes, there's not much code that deals with the "item" (and its nomenclature) at the individual level. That's pretty much just limited to internal field(_ui).module code and normalizers for HAL and other serialization formats.
• However, lots of code deals with the set of items: widgets, formatters, theme templates and preprocess hooks, and everything else dealing with the data in a field of a given entity. For that, FieldValueInterface seems more natural and grokkable than FieldItemListInterface.
• Since FieldItemList* doesn't exist as a name in HEAD yet, there's no more work or disruption to picking FieldValue* as the name instead.

"FieldValueListInterface $values", "$values[0]->getValue()", "A list of values that consist of values", really? :) And I thought ->value vs. getValue() is bad and ambiguous, but seems harmless compared to this.

So instead, we'd have maybe something like FieldValueInterface $field_value, which would lead to occasional code like $field_value[0]->getValue() and $field_value[0]->value. The first shouldn't be that common: there's not many places where it's a good idea to convert from object to array; but if we're concerned about those few places, we can add a method named toArray() and use that instead. The second is a little awkward, and basically assumes that you get used to the idea that a field value is a set of items, where each item can contain one or more properties, and one of those properties can be, and often is, named "value". But that's pretty much the foundational bit of knowledge you need to have about Drupal's data model regardless.

FWIW, I would prefer to keep the "item" term because saying "the value of a field value" is just confusing as hell :/

Tagging.

#17: field-item-list-2051923-17.patch queued for re-testing.

I'd prefer to use FieldItems and FieldItemsInterface because FieldItemList is a kind of type of item and a bit better then it's now.

PS why not EntityProperty that contains items

Also filed #2089807: Rename Field::fieldInfo() to FieldService::fieldInfo() for better DX

Re-roll of the existing patch, that was ugly. Whatever we'll end up with, it will be easier to update an applying patch.

Missed one, maybe that caused the install fail?

...

This should be green again.

The conclusion from yesterdays meeting was:

• The API change is ok and would provide a good DX improvement
• Drupal\Core\Entity\Field\FieldInterface > Drupal\Core\Entity\Field\FieldItemListInterface
• Drupal\Core\Entity\Field\FieldItemInterface > stays the same
• Drupal\text\Plugin\field\field_type\TextItem (and the like) > Drupal\text\Plugin\field\field_type\TextFieldItem

The rest of the renaming we discussed should probably be covered by #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'.

Here's a simple reroll. The current patch lives up to what we agreed on yesterday.
The only thing I missed in our notes was around ""

Nit-pick fix:

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -19,14 +19,14 @@
  * containing fields as its data properties. The contained fields have to
- * implement the \Drupal\Core\Entity\Field\FieldInterface, which builds upon
+ * implement the \Drupal\Core\Entity\Field\FieldItemListInterface, which builds upon
  * typed data interfaces as well.

Fixed comment that crossed 80 characters.

Here's a patch including the actual file renames too *doh*

Let's try again, shall we...

#2094003: Remove use statements in formatters, widgets and field types plugins is postpone on this.

80 chars

Fixed.

Arguably all of those should be {@inheritdocs} now ?

Fixed.

diff --git a/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php b/core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php
similarity index 94%
rename from core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileField.php
rename to core/modules/file/lib/Drupal/file/Plugin/field/field_type/FileFieldItemList.php

I guess this file rename serves as a demonstration of how the field item classes should be renamed. Should we open up separate issues for renaming the rest of the field item classes (i.e. TextItem, TelephoneItem etc?

I'm actually not sure if we agreed on renaming the item classes or not, but we certainly should postpone that to a separate issue.

@Berdir Ok. Let's postpone renaming the item classes to a separate issue.

None of the item classes are renamed in this patch, so this should be good to go, imo.

I reviewed the patch, but I was not able to find any glitches.

I noted some existing field API docs document $items as field value partly already, but given the documented FieldItemList interface and $items clarifies which values that are exactly, this is not really a problem.

So imo this is ready.

Rerolled

Rerolled

#57: 2051923-field-naming-56.patch queued for re-testing.

Giving it a better title.

Lets update the issue summary to reflect the current patch and assigning to Dries as he expressed an opinion earlier in the issue.

The patch looks good to me and implements what was agreed in Prague.

Fixed incorrect namespace of existing FieldInterface and FieldInstanceInterface within field.module.

Updated issue summary.

Updated issue summary.

#57: 2051923-field-naming-56.patch queued for re-testing.

Updated issue summary.

Updated issue summary.

Re-roll after the comment patch got in (yay!).

Almost no conflicts, just updated the widget and formatter classes, and conflicted because it removed an unrelated @todo in the Comment class ;)

#62: 2051923-field-naming-62.patch queued for re-testing.

#62: 2051923-field-naming-62.patch queued for re-testing.

#62: 2051923-field-naming-62.patch queued for re-testing.

changes are good, so back to RTBC

Committed 510c81a and pushed to 8.x. Thanks!

Fixed a couple of things during commit...

diff --git a/core/modules/comment/lib/Drupal/comment/Entity/Comment.php b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
index f7e6830..617d4ad 100644
--- a/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -79,7 +79,7 @@ class Comment extends EntityNG implements CommentInterface {
   /**
    * The entity ID for the entity to which this comment is attached.
    *
-   * @var \Drupal\Core\Entity\Field\FieldInterface
+   * @var \Drupal\Core\Entity\Field\FieldItemListInterface
    */
   public $entity_id;

@@ -93,7 +93,7 @@ class Comment extends EntityNG implements CommentInterface {
   /**
    * The field to which this comment is attached.
    *
-   * @var \Drupal\Core\Entity\Field\FieldInterface
+   * @var \Drupal\Core\Entity\Field\FieldItemListInterface
    */
   public $field_id;

@@ -181,16 +181,6 @@ class Comment extends EntityNG implements CommentInterface {
   public $thread;

   /**
-<<<<<<< HEAD
-=======
-   * The comment node type.
-   *
-   * @var \Drupal\Core\Entity\Field\FieldItemListInterface
-   */
-  public $node_type;
-
-  /**
->>>>>>> applied patch
    * Initialize the object. Invoked upon construction and wake up.
    */
   protected function init() {
diff --git a/core/modules/field/lib/Drupal/field/FieldInterface.php b/core/modules/field/lib/Drupal/field/FieldInterface.php
index c4ccd0e..2a03aab 100644
--- a/core/modules/field/lib/Drupal/field/FieldInterface.php
+++ b/core/modules/field/lib/Drupal/field/FieldInterface.php
@@ -2,7 +2,7 @@

 /**
  * @file
- * Contains \Drupal\field\FieldItemListInterface.
+ * Contains \Drupal\field\FieldInterface.
  */

 namespace Drupal\field;

Follow-up, to clean-up the rest

Ups.

More clean

John@JOHN-PC /d/apache/drupal8/core (8.x)
$ grep -r 'Drupal\\Core\\Entity\\Field\\Field\>' .
./lib/Drupal/Core/Entity/Field/FieldItemList.php: * Contains \Drupal\Core\Entity\Field\Field.
./modules/system/entity.api.php: * - field: The entity field object (\Drupal\Core\Entity\Field\Field).

I'm already handling that in #1994140: Unify entity field access and Field API access, except the additional use's, should we do that there?

Committed and pushed to 8.x. Thanks!

Works too ;)

Re-opening for the change notice.

another cleanup #2100411: Wrong doc for interface EntityInterface.

Created https://drupal.org/node/2101747, which is based on the issue summary.

Not sure about this one, wanted to discuss this with @xjm. This is a HEAD to HEAD change, so we don't really need a change notice for that part. I was planning on just updating some existing ones about @FieldType's to include documentation about the list_class, which currently isn't documented.

Simplified the change notice as discussed with @webchick, also updated https://drupal.org/node/2064123 to mention the list classes.

Removing sprint tag.