Updated: Comment #61

Problem/Motivation

Different things and classes are currently called Field.
- The field config entity that contains the basic field definition (Drupal\field\Entity\Field)
- The field objects on the entity that contains the field items and their values (Drupal\Core\Entity\Field\Field)

There are also two more classes that are called entity:
- \Drupal\field\Field which is used to access the field info service (Field::fieldInfo())
- A views field class to display fields.

There were also discussions around the FieldItem class and what exactly "field values" are.

Proposed resolution

After *long* discussions at the #HardProblems sprint on sunday, we agreed on the following terminology and class names:

- We figured the only confusing term right now is "Field", while we field items and field item properties is not confusing to Drupal folks.
- The suggestion of going with "field values" instead of "field items" was discussed. However, "values" would apply to each layer (field item list, field items, field item properties) so is ambiguous. Contrary, item has quite a Drupal history is all over the place, specific and clear, so we stay with that.
- The Field class on entities is a list of FieldItem's, so will be renamed to FieldItemList, FieldItemListInterface and also all subclasses accordingly. A common way to refer to it is "FieldItemListInterface $items". FieldItems was discussed, but was considered to be hard to distinguish from FieldItem This is what this patch does.
- The meaning of what a "field" is depends on the context. If the context is clear, it's ok to refer to it as field/$field. If the context is not clear, e.g. when it's about the field definition, $field_definition/FieldDefinition should be used.
- Therefore, FieldDefinitionInterface, FieldDefinition and related methods will not be renamed.
- The static Field class will be renamed in #2089807: Rename Field::fieldInfo() to FieldService::fieldInfo() for better DX
- The Field (field_entity) will be renamed to FieldConfig (field_config), FieldInstance (field_instance) will be renamed to FieldInstanceConfig (field_instance_config) #2095303: Rename 'field_entity' to 'field_config' and 'field_instance' to 'field_instance_config'

Class overview after the rename:

Remaining tasks

Committing it!

User interface changes

None.

API changes

As outlined above. \Drupal\Core\Entity\Field\Field => FieldItemList, \Drupal\Core\Entity\Field\FieldInterface => FieldItemListInterface

All other API changes will be done in other issues.

See above.

Original report by @yched

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)

The initial direction (see #2019601: Rename config Field / FieldInstance structures ?) was to keep "field" for the values, and rename definitions to something else, but:
- this approach has been shot down after a while - see #2019601-72: Rename config Field / FieldInstance structures ?
- turns out EntityNG intends to rename the "value objects" from Field to FieldItemList anyway - #1869574: Support single valued Entity fields

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

If "field" stays "a field definition", then:
- it seems it would make sense to rename the "generic" Drupal\Core\Entity\Field\FieldDefinitionInterface used by widgets and formatters, to simply FieldInterface.
- if so, then this clashes with Drupal\field\Plugin\Core\Entity\FieldInterface (the interface for the "configurable field" ConfigEntity : Field) - and one interface extends the other :-).
So we'd still need to rename Field API's config entity classes for "definitions of configurable fields".

Proposal

Note: this proposal should imply very little variable renames, which is definitely a good thing.

- Abstract "field definition" interface added by #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets:
Drupal\Core\Entity\Field\FieldDefinitionInterface --> FieldInterface

- "field definition" class for "base fields" being added at #2047229: Make use of classes for entity field and data definitions:
Drupal\Core\Entity\Field\FieldDefinition (current name in the patch) --> Field ? BaseField ?

- "configurable field" ConfigEntities handled by field.module:
Drupal\field\FieldInterface --> ConfigurableFieldInterface
Drupal\field\FieldInstanceInterface --> ConfigurableFieldInstanceInterface
Drupal\field\Plugin\Core\Entity\Field --> ConfigurableField - entity_type 'field' - CMI: field.field.*
Drupal\field\Plugin\Core\Entity\FieldInstance --> ConfigurableFieldInstance - entity_type 'field_instance' - CMI: field.instance.*
no need to rename existing $field / $instance variables, which was probably 80% of the huge patch in #2019601: Rename config Field / FieldInstance structures ?.

- Field values:
Drupal\Core\Entity\Field\Field --> FieldItemList (as per #1869574: Support single valued Entity fields)
Drupal\Core\Entity\Field\FieldInterface --> FieldItemListInterface
Drupal\Core\Entity\Field\FieldItem --> unchanged
Drupal\Core\Entity\Field\FieldItemBase --> unchanged
Drupal\Core\Entity\Field\FieldItemInterface --> unchanged
FieldItemList/FieldItemBase::getFieldDefinition() --> getField() (returns a FieldInterface : "abstract field definition")

- "Configurable field" values:
Drupal\field\Plugin\Type\FieldType\ConfigField -> ConfigurableFieldItemList (that class might turn useless after #2047229: Make use of classes for entity field and data definitions)
Drupal\field\Plugin\Type\FieldType\ConfigFieldInterface -> ConfigurableFieldItemListInterface (same)
Drupal\field\Plugin\Type\FieldType\ConfigFieldItemBase -> ConfigurableFieldItemBase
Drupal\field\Plugin\Type\FieldType\ConfigFieldItemInterface -> ConfigurableFieldItemInterface

Files: 
CommentFileSizeAuthor
#73 rename_field-2051923-73.patch2.64 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rename_field-2051923-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#73 interdiff-71-73.txt1.12 KBsmiletrl
#71 drupal8.entity-system.2051923-71.patch1.52 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,186 pass(es).
[ View ]
#62 2051923-field-naming-62.patch135.92 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]
#62 2051923-field-naming-62-interdiff.txt2.02 KBBerdir
#57 2051923-field-naming-56.patch133.65 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,643 pass(es).
[ View ]
#56 2051923-field-naming-56.patch133.65 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,972 pass(es).
[ View ]
#52 2051923-field-naming-51.patch133.63 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 59,062 pass(es).
[ View ]
#49 2051923-field-naming-49.patch133.7 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 58,545 pass(es).
[ View ]
#49 interdiff.txt905 bytesdixon_
#48 2051923-field-naming-48.patch133.5 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#48 interdiff.txt905 bytesdixon_
#46 2051923-field-naming-46.patch132.51 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#46 interdiff.txt905 bytesdixon_
#44 field-item-list-2051923-44.patch133.67 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,814 pass(es).
[ View ]
#44 field-item-list-2051923-44-interdiff.txt815 bytesBerdir
#42 field-item-list-2051923-42.patch133.57 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,613 pass(es), 8 fail(s), and 5 exception(s).
[ View ]
#42 field-item-list-2051923-42-interdiff.txt847 bytesBerdir
#40 field-item-list-2051923-40.patch133.47 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,233 pass(es), 7 fail(s), and 5 exception(s).
[ View ]
#40 field-item-list-2051923-40-interdiff.txt681 bytesBerdir
#38 field-item-list-2051923-38.patch133.67 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#38 field-item-list-2051923-38-interdiff.txt1.3 KBBerdir
#36 field-item-list-2051923-35.patch132.37 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#36 field-item-list-2051923-35-interdiff.txt721 bytesBerdir
#34 field-item-list-2051923-34.patch132.03 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#17 field-item-list-2051923-17.patch137.1 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-item-list-2051923-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 field-item-list-2051923-17-interdiff.txt683 bytesBerdir
#13 fielditemlist.png16.5 KBBerdir
#13 field-item-list-2051923-13.patch136.74 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,349 pass(es), 22 fail(s), and 11 exception(s).
[ View ]

Comments

yched’s picture

Issue summary:View changes

typo

yched’s picture

Comment from @plach copied over from #2019601-79: Rename config Field / FieldInstance structures ?:

I'd say we definitely want to do this, it looks like an obvious DX win.
+1 for method renames, not sure about the related stuff in the Typed Data API. It would be cool to get rid of the property terminology once and for all.

plach’s picture

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

yched’s picture

@plach - yup, sorry. I'm not sure I agree and would welcome other opinions, but I didn't consciously intend to wipe out yours ;-)

yched’s picture

Issue summary:View changes

variable renames...

effulgentsia’s picture

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.

effulgentsia’s picture

yched’s picture

the reason for this is to disambiguate Drupal\field\FieldInterface from Drupal\Core\Entity\Field\FieldInterface, but do we really need that disambiguation?

I'd think it is fairly important, yes - it's fairly important IMO to differentiate code that works with "any field / the abstract definition interface" (should be the common case), and code that still strictly expects "configurable fields" specific structures (should be pretty rare - at least that's what we're aiming for).

People come from $field and $instance in D7. The fact that there are "other kinds of fields" and that there is a common interface is a sufficiently important novelty that the "new generic interface" and "the interface for the familiar $field objects" shouldn't have the exact same name IMO.

webchick’s picture

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.

plach’s picture

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
yched’s picture

@webchick: "but "FieldItemList" is problematic"
Can you expand ? :-)

Actually, it's the discovery that @fago was in the process of renaming Drupal\Core\Entity\Field\Field to FieldItemList in #1869574: Support single valued Entity fields, along with separate conclusion that settling on "Field" for field values rather than field definitions was a no-go, that triggered this proposal... (see #2019601-72: Rename config Field / FieldInstance structures ?

@plach:
"it seems we are using the configurable terminology as a synonym of "field defined by the Field API module"
Uh ? Most definitely, that's what the definition of a "configuration field" is.
"However theoretically any module can define a configurable field"
Fields live either in code (StorageController::baseFieldDefinitions()) or in configuration (and they are then managed by field.module's ConfigEntities). I never heard that we intended to support a 3rd kind ?

plach’s picture

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?

msonnabaum’s picture

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.

yched’s picture

Sorry for not following through earlier :-/.
@fago himself seems to want to move away from "Field" for "values", so I'm a little frustrated to see what seemed to be a consensus (and a hard to reach one) from the involved parties questioned again - but well, I'm biased :-)

I don't see that FieldDefinition disrupts our language much.

I can only repeat what I posted in #2019601-72: Rename config Field / FieldInstance structures ?:

- In practice, it makes more convoluted sentences in code comments, documentation...
- It disrupts years of habits and online/offline drupal litterature - $field since CCK 4.7 has always been about "the definition structure", not the "the values in an entity".
- More importantly, we cannot promote disruption by saying it's a good move, because it's not inline with actual use in common language, for example when discussing architecture when building a site:
"this is a text field"
"this would require a new field"
"should we share this field or create a new one ?"
- When looking at "entities" (general, non-drupal sense), one key aspect in defining and naming concepts is answering the question "Is this the same 'thingie', are those different 'thingies' ?".
If 'field' means "the value of a given 'field definition' in a given entity" (see, even that sentence is horrid :-)), then "Is this the same field ?" just doesn't mean anything, it will never be "the same field". When talking about the definitions, then "It's the same field definition" feels ambiguous, conveys equality but not really identity.
If "field" means "the definition", as it did so far, then "is this the same field ?" has a meaning - and it's a sentence we've all been using in our day-to-day life as drupal devs or site builder.

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

Those examples apply only if you already consider "Field" to be "the value objects you find in entity objects".
I could make the same arguments with "Field" being "the definition objects". If you want to instantiate one, you end up with an object that implements FieldInterface ?

We're not asking this of any other API. A "View" is not a "view definition". A "text format" is not a "text format definition".
This really adds a non-minor maintenance overhead
- rewriting comments (and again, this is not just a search / replace, non trivial english refactoring involved, and produces sentences that look weird / ambiguous),
- maintaining code with variables named $field_definition / $instance_definition (or more lengthy flavors)
- makes naming things much more convoluted... We have base fields and configurable fields, that are based on a $field / $instance dichotomy.
So... ConfigurableFieldDefinition ? ConfigurableFieldInstanceDefinitionInterface ?
"configurable field definition" feels just weird - looks like it's the "definition" that is configurable... Same thing in lots of similar sentence configurations...

My point is, using "field" for values puts the benefit of "short, flexible, unambiguous term" at the wrong place IMO.
- Most of our core code base, all of (D7) contrib & custom code and docs use "field" for the definitions. So this is a complete turn around
- for no good reason IMO, since it makes our lives more difficult...

Then again - we can hardly reach any agreement without @fago chiming here...

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new136.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,349 pass(es), 22 fail(s), and 11 exception(s).
[ View ]
new16.5 KB

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:
fielditemlist.png

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

Berdir’s picture

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

Status:Needs review» Needs work

The last submitted patch, field-item-list-2051923-13.patch, failed testing.

effulgentsia’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new683 bytes
new137.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-item-list-2051923-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should fix those test failures.

yched’s picture

@Berdir: thanks !!!

If I get things right, the patch roughly does the "Field values" part of the OP ?
What do you think about the other parts of the proposal ?

Berdir’s picture

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.

webchick’s picture

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.

Berdir’s picture

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?

effulgentsia’s picture

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.

effulgentsia’s picture

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.

yched’s picture

I haven't fully considered all the implications of FieldValue / FieldValueList yet - but even without delving into that, we have to acknowledge that this is a *far* more disruptive and far-reaching change than what was proposed in the OP.

- As @Berdir mentions, FieldItem is already widely present in core, along with a lot of $item / $items variables, because that was the typical name used way back from CCK...
- It doesn't conflict with anything right now, so renaming that, even if possibly leading to a clearer final state with class names matching "natural language", it isn't strictly needed to solve the "Field" WTF.
- TypedData has ItemList, which FieldItemList in the proposal from the OP (and in @fago's patch at #1869574: Support single valued Entity fields) extends.
So that would be "Field/FieldValueList extends TypedData/ItemList" - acceptable I guess, just pointing that out.

I don't necessarily disagree with the idea right now, I kind of like it in fact, but I do fear the derailing effect it might have in this issue...
Also, that puts us back to "can we move without @fago's approval ?", while he was implicitly ok with FieldItemList (since he's the one that first came with it)

Berdir’s picture

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

effulgentsia’s picture

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.

yched’s picture

I'm afraid FieldValue as "the list of items" doesn't work, because it disrupts common language around "mono valued field, multiple values, number of values..." - used heavily in real-life discussions, UI, Drupal litterature & blog posts, as well as in our code comments... On a field with cardinality > 1, I'd say the huge majority of people refer to "a value" as $node->field_foo[$delta].

And yes, I do realize that the above is another argument in favor of stopping the "natural language / technical language" gap and unify on FieldValueList / FieldValue :-/.

What I mean here is, *if* we introduce the word "value" somewhere, IMO it has to point to what are currently named Item, not to the list of items.

amateescu’s picture

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

Berdir’s picture

Issue tags:+sprint, +Entity Field API

Tagging.

Berdir’s picture

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

The last submitted patch, field-item-list-2051923-17.patch, failed testing.

andypost’s picture

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

andypost’s picture

Berdir’s picture

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

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

Status:Needs review» Needs work

The last submitted patch, field-item-list-2051923-34.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new721 bytes
new132.37 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Missed one, maybe that caused the install fail?

Status:Needs review» Needs work

The last submitted patch, field-item-list-2051923-35.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new1.3 KB
new133.67 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Status:Needs review» Needs work

The last submitted patch, field-item-list-2051923-38.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new681 bytes
new133.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,233 pass(es), 7 fail(s), and 5 exception(s).
[ View ]

...

Status:Needs review» Needs work

The last submitted patch, field-item-list-2051923-40.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new847 bytes
new133.57 KB
FAILED: [[SimpleTest]]: [MySQL] 58,613 pass(es), 8 fail(s), and 5 exception(s).
[ View ]

This should be green again.

Status:Needs review» Needs work

The last submitted patch, field-item-list-2051923-42.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new815 bytes
new133.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,814 pass(es).
[ View ]
dixon_’s picture

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

dixon_’s picture

StatusFileSize
new905 bytes
new132.51 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 2051923-field-naming-46.patch, failed testing.

dixon_’s picture

Status:Needs work» Needs review
StatusFileSize
new905 bytes
new133.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

dixon_’s picture

StatusFileSize
new905 bytes
new133.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,545 pass(es).
[ View ]

Let's try again, shall we...

yched’s picture

Awesome. Aside from a minor 80 chars wrapping error, this looks good to me :-)

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
    @@ -13,7 +13,7 @@
    + * for a particular entity (see \Drupal\Core\Entity\Field\FieldItemListInterface). For

    80 chars

  2. +++ b/core/lib/Drupal/Core/Entity/Field/FieldItemList.php
    @@ -148,49 +146,49 @@ public function setValue($values, $notify = TRUE) {
    -   * Implements \Drupal\Core\Entity\Field\FieldInterface::getPropertyDefinition().
    +   * Implements \Drupal\Core\Entity\Field\FieldItemListInterface::getPropertyDefinition().

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

jibran’s picture

dixon_’s picture

StatusFileSize
new133.63 KB
PASSED: [[SimpleTest]]: [MySQL] 59,062 pass(es).
[ View ]

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?

Berdir’s picture

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.

dixon_’s picture

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

fago’s picture

Status:Needs review» Reviewed & tested by the community

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.

plach’s picture

StatusFileSize
new133.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,972 pass(es).
[ View ]

Rerolled

plach’s picture

StatusFileSize
new133.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,643 pass(es).
[ View ]

Rerolled

Berdir’s picture

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

Berdir’s picture

Title:Settle naming around "Field" classes» Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList

Giving it a better title.

alexpott’s picture

Assigned:Unassigned» Dries
Issue tags:+Needs issue summary update

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.

alexpott’s picture

Issue summary:View changes

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

Berdir’s picture

Issue summary:View changes

Updated issue summary.

Berdir’s picture

Issue summary:View changes

Updated issue summary.

Berdir’s picture

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

Berdir’s picture

Issue summary:View changes

Updated issue summary.

fago’s picture

Issue summary:View changes

Updated issue summary.

Berdir’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.02 KB
new135.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -API change, -sprint, -Entity Field API, -Approved API change

The last submitted patch, 2051923-field-naming-62.patch, failed testing.

fago’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 2051923-field-naming-62.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 2051923-field-naming-62.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Needs issue summary update, +API change, +sprint, +Entity Field API, +Approved API change

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

fago’s picture

Status:Needs review» Reviewed & tested by the community

changes are good, so back to RTBC

alexpott’s picture

Title:Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList» Change notice: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList
Status:Reviewed & tested by the community» Active

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;

andypost’s picture

Status:Active» Needs review
StatusFileSize
new1.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,186 pass(es).
[ View ]

Follow-up, to clean-up the rest

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Ups.

smiletrl’s picture

StatusFileSize
new1.12 KB
new2.64 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch rename_field-2051923-73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Berdir’s picture

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?

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Berdir’s picture

Status:Fixed» Active

Works too ;)

Re-opening for the change notice.

smiletrl’s picture

smiletrl’s picture

Status:Active» Needs review

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

Berdir’s picture

Status:Needs review» Needs work

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.

Berdir’s picture

Title:Change notice: Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList» Rename \Drupal\Core\Entity\Field\Field, related subclasses and interfaces to *FieldItemList
Status:Needs work» Fixed
Issue tags:-Needs issue summary update

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

webchick’s picture

Assigned:Dries» Unassigned
Berdir’s picture

Issue tags:-sprint

Removing sprint tag.

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.