The whole point of
#1969728: Implement Field API "field types" as TypedData Plugins,
#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets,
and more generally #1949932: [META] Unify entity fields and field API,
has been to unify base fields and configurable fields by:
- having the same set of supporting code (field type classes)
- then being able to use applicable widgets and formatters on them based on those field types.

We have everything in place to do that, and in the conversions from "old Field API field types to new EntityNG field types" currently under way, we make sure that field type classes are agnostic about "base or configurable" fields and can be used on both.

But we need to decide who will provide those "field type" classes if we want to use them for base fields in core entity types.

Take email field, for example. In current HEAD, there is:
- Drupal\Core\Entity\Plugin\DataType\EmailItem (provided by Core/Entity), exposed as a the 'email_field' DataType plugin.
This is used for the 'mail' "base field" of contact.module's Message entities (but oddly enough not for User 'mail' for now)
- email.module, that still uses the legacy hook_field_info() to expose the 'email' FieldType, which is then derived to the 'field_type:email' DataType.
Moving this to the new "field type API" means exposing a new class implementing FieldItemInterface, but also ConfigurableFieldItemInterface so that this can be used as a "configurable field".

How do we do that ?

1) email.module provides ConfigurableEmailFieldItem, extending Core's EmailItem ?
This is what #2015703: Convert field type to typed data plugin for email module currently does (see #19-#21 over there).

Problem:
We bloat our "field type plugins" registry with two separate field types, one for base fields, the other for configurable fields.
Then widgets & formatters need to explicitly account for both ("I work on 'email' and 'configurable_email' field types").
Repeat that for all the field types we might want to use in base fields (text, number, maybe even link and entity_ref...).
This is kind of silly, since the only thing in ConfigurableEmailFieldItem is the schema() method. One single class / one single field type could totally work indifferently on base fields and configurable fields. That's the whole point of the abstract FieldDefinitionInterface.
If "two separate sets of field types for base or configurable fields" is where we end up, I'd say we haven't unified anything...

2) Remove Drupal\Core\Entity\Plugin\DataType\EmailItem,
email.module provides Drupal\email\Plugin\field\field_type\EmailItem as the 'email' field type,
The 'mail' field in Message entities 'mail' field uses 'type' => 'field_type:email'

This means contact.module has a dependency on email.module - or more generally, modules providing entity types require the modules that provide the field types they want to use for their base fields. Well, this only seems reasonable?

Problem:
Are we OK with having contact.module, node.module, user.module... require text.module, number.module, email.module, maybe entity_ref.module... ?
Means those modules are de-facto required on all sites.

3) Remove email.module,
add everything it takes to be used for configurable fields in the EmailItem provided in Drupal\Core\Entity.

This means shipping "basic" field types in the Drupal\Core package rather than in standalone modules.
Granted, once the field type, the widgets, the formatters are in plugin classes, there's not much code left in email.module, number.module, etc. There's a little more in text.module, and much more in entity_reference.module.

Problem:
- Where is the line ? is "text" basic ? what about "file", "image", "entity_ref" ?
- This means Drupal\Core has classes that extend ConfigurableFieldItemBase, which is currently provided by field.module
- Then we also need to move along the widgets and formatters plugins provided by the former separate modules ?
How do we organize that ? The annotation discovery forces "all field types in a folder, all formatters in a folder, all widgets in a folder". Given that some of them use base classes, that's a lot of classes blended together...

Comments

yched’s picture

Right now, I hate option 1) with a passion, and I'd say option 2) is still the path of least resistance...

yched’s picture

Issue summary:View changes

formatting

yched’s picture

Issue summary:View changes

language

yched’s picture

Issue summary:View changes

more specific

yched’s picture

Issue summary:View changes

bla

amateescu’s picture

Another problem that we have with option 1), besides it being far from ideal, is actually the schema() method that is currently the only difference between the two "email" plugins.

Right now, we're pretending that "base" fields don't have (or need) a schema, but they do, it's just burried in the hook_schema() implementation that defines the base/data/revision tables of the entity type. I know @plach wanted us to get to a state where we could generate entity tables dynamically by introspecting the base field definitions, and I think we should still aim for this in D8.

tstoeckler’s picture

You elude to a problem with 2. in the first part of the OP: user.module should be using the email field for user accounts, so with 2. user.module would have to depend on email.module, which would make email.module required.

I don't really see the downside of 3. TBH. My personal answers to your problems/questions about that approach would be:

- Where is the line ? is "text" basic ? what about "file", "image", "entity_ref" ?

I think the question should be the other way around: What makes something a module? Possible answers: The need for a schema, shipped configuration, hooks need to implemented, ...
None of these apply here so put it in Drupal\Core! :-)

- This means Drupal\Core has classes that extend ConfigurableFieldItemBase, which is currently provided by field.module

That is very unfortunate but really all those classes should not be in field.module but in \Drupal\Core\Entity\Field anyway. This would be very much in-line with the effort to remove field.module in favor of entity.module / \Drupal\Core\Entity

Then we also need to move along the widgets and formatters plugins provided by the former separate modules ?
How do we organize that ? The annotation discovery forces "all field types in a folder, all formatters in a folder, all widgets in a folder". Given that some of them use base classes, that's a lot of classes blended together...

This is a valid concern, but not something that should hold this up, in my opinion. Plugins have the concept of a 'provider', so we basically have 3 options here:
a) Decide that the entity system (\Drupal\Core\Entity) is the provider of all those fields, which would cause the problem you described.
b) Decide that the (currently non-existing) 'email component' (\Drupal\Core\Email) is the provider for the email field, and likewise for all other fields, which would do away with the crowded directories but also lose the nice namespacing of everything under \Drupal\Core\Entity
c) Somehow make the Plugin system more flexible, so that it finds stuff in \Drupal\Core\Entity\Email, etc.

yched’s picture

You elude to a problem with 2. in the first part of the OP: user.module should be using the email field for user accounts, so with 2. user.module would have to depend on email.module, which would make email.module required.

I don't elude that, it's mentioned in the OP :) :
"Are we OK with having contact.module, node.module, user.module... require text.module, number.module, email.module, maybe entity_ref.module... ? Means those modules are de-facto required on all sites"

I think the question should be the other way around: What makes something a module? Possible answers: The need for a schema, shipped configuration, hooks need to implemented, ...
None of these apply here so put it in Drupal\Core! :-)

Well, text.module still implements some hooks (react to "text format" config entities changing)
entity_ref.module too - and has *lots* of supporting code.
image, file, taxo, even more.
So, yes, "where is the line ?", which field types deserve to be in Core\Entity, which ones get separated in their own modules ?

They can also have settings in CMI files (text does), possibly update hooks... This means those all go in entity.module ?
I'm not super comfortable with plugins provided in lib/drupal/core, and their "moduley" pieces handled in entity.module.

Also, this pattern only works for core. Contrib field types will still be shipped in modules, and contrib entity types that want to use them for their base fields will need to require them, that's unavoidable.
Not necessarily a big problem, it's just a double standard.

really all those classes should not be in field.module but in \Drupal\Core\Entity\Field anyway. This would be very much in-line with the effort to remove field.module in favor of entity.module / \Drupal\Core\Entity

We're talking about moving the "support user-defined fields" functionality to the Core\Entity component / entity.module.
It's not something I'm necessarily opposed to, but that's not a minor change, and it can easily take us weeks to get there.

I still have the feeling that option 2) is the shortest path right now, even if we reconsider later.

tstoeckler’s picture

@yched: Sorry, I thought "to elude" meant something else. That didn't make any sense, the way I said it.

I'm not super comfortable with plugins provided in lib/drupal/core, and their "moduley" pieces handled in entity.module.

Yes, I agree that would be an unnatural split. I wasn't aware of all the "moduley" pieces you mention in #4. That certainly convinces me that 2. is the way to go. I'm not sure we can really live with

"Are we OK with having contact.module, node.module, user.module... require text.module, number.module, email.module, maybe entity_ref.module... ? Means those modules are de-facto required on all sites"

, however. Required modules really suck... Again, user.module is still required and most likely will be in Drupal 8. So that makes at least text.module, email.module and date.module required as well. entity_reference.module would then be required by node.module "only".

smiletrl’s picture

Path 1) looks good to me:)

a). For path 3) and path 2), the unified ItemClass needs work for both, i.e., base fields and configurable fields. And one should depends on the other. This could confuse developers. For example, if I only need to provide a base field field_type in contrib module, this field_type plugin needs to implement ConfigFieldItemInterface too! This doesn't make sense to me:(

Despite this bundle of base field and configurable field, they do have different things that should be highlighted in their own ItemClasses or plugin annotation. For example schema(), and property 'is_configurable'.

EmailItem here looks like a happy coincident:) For other base field Item at this moment, e.g., 'IntegerItem', 'LanguageItem', 'StringItem', they don't implement 'ConfigFieldItemInterface' as EmailItem too. So I guess no need to enforce these base field items to implement 'ConfigFieldItemInterface' too, just for some unifying stuff?

b). I guess path 1) doesn't break the original purpose of 'FieldDefinitionInterface'. This interface is introduced to unfiy base fields and configurable fields in some sense, so they could be treated as the same in certain circumstances, e.g., field formatter needs to work for both, so they should be treated the same.

In the field item class context here, looks like they don't have to be the same. And they work perfectly with each own item class here. Eveything is clear to developer, that one field_type is not configurable, so it should only be used for base fields, and it shouldn't implement 'ConfigFieldItemInterface', which is only for configurable fields.

For the formatter and possible widget stuff, there would be some 'repeat' field_types perhaps. Like, date formatter and email formatter, they could need to declare they support both date(node.created) and configurable_date(complex date field) field_type. But for most field_types, they are clearly un-connected. For StringItem as base field(node.title) now, text formatter perhaps needs to declare it supports field_type 'Text' and 'String' too.

In a word, merits of path 1) surpass its shorcomings imho:)

yched’s picture

the unified ItemClass needs work for both, i.e., base fields and configurable fields. And one should depends on the other. This could confuse developers.

I don't get this. What would depend on what ?

For example, if I only need to provide a base field field_type in contrib module, this field_type plugin needs to implement ConfigFieldItemInterface too

No, you can totally provide an Item class that doesn't implement ConfigFieldItemInterface and use it for your base fields. It just won't be shown in Field UI's "add new field".

This discussion is not about *enforcing* Item classes to implement ConfigFieldItemInterface. It's only about limiting useless duplication in the field types shipped in core, for clarity, maintainability, promotion of best practices, memory clutter...
Contrib/custom modules can do whatever they see fit.

So, unless I understood wrong, I think #6 is moot, and still hate option 1) and favor option 2) :-)

And yes, this does mean core's 'IntegerItem', 'StringItem', would be better off unified/merged with the "configurable" text and number field types.

smiletrl’s picture

Right, right, I took it the wrong way:) Contrib module doesn't have to implement ConfigFieldItemInterface for base field, but that's what Drupal core does, if we use option 2).

So does this make base field object a little bigger than it should be? For instance, StringItem should be relatively simple, but since it's using TextItem provided by Text module, String base field object could contain methods, like 'instanceSettingsForm', 'settingsForm', etc, which probably never be used for base fields. Does this make php-memory cost much when it comes to base field object?

Also, this probably adds difficulty for base field's FieldDefinition to extract field definition at #2047229: Make use of classes for entity field and data definitions. Configurable field's Field Definition has already highly depended on the field_type plugin/Field Item Class. For instances, configurable field's definition method -- FieldDefinitionInterface::getFieldSetting will count 'settings', and 'instance settings' in plugin annotation. What should base field's fieldSettings count? Only 'settings', but not 'instance settings'?

Ok, it's just a bit weird to create base field Item from configurable field item. As base field item are originally supposed to be a simple piece of data:)

yched’s picture

So does this make base field object a little bigger than it should be? For instance, StringItem should be relatively simple, but since it's using TextItem provided by Text module, String base field object could contain methods, like 'instanceSettingsForm', 'settingsForm', etc, which probably never be used for base fields. Does this make php-memory cost much when it comes to base field object?

Yes, field item classes would have more code to support "can be used for configurable fields". I don't think that's a real issue, it's just like 50/100 lines of code that's not executed if the class is used for a base field. Much better that duplicate classes polluting our plugins registries and the widgets / formatters ecosystem.

Also, this probably adds difficulty for base field's FieldDefinition to extract field definition at #2047229: Make use of classes for entity field and data definitions

I don't see why. That's the whole premise of FieldDefinitionInterface. Same interface, different implementation classes depending on where the field definition comes from (config or code).

What should base field's fieldSettings count? Only 'settings', but not 'instance settings'?

Exactly. Base fields have no 'instances', they appear on every bundle on the node type.

Ok, it's just a bit weird to create base field Item from configurable field item. As base field item are originally supposed to be a simple piece of data:)

Don't mix the data ("field properties") with the class methods. An EmailFieldItem and a ConfigurableFieldItem have exactly the same data (an 'email' property) with the same semantics (it holds a mail address) - which is why duplication is meaningless and makes it more complex for widgets and formatters, that need to expose themselves for both types while they do the exact same thing with the values.
ConfigurableFieldItem just has more methods because it has more features (can be created in the UI). that doesn't make it weird.

fago’s picture

I must say that I don't think option 1) is so bad. I'd like having simpe field types provided by Core, and then modules adding more complex ones.

We bloat our "field type plugins" registry with two separate field types, one for base fields, the other for configurable fields.

As to that, couldn't we just use a alter hook to swap out the plugin class and make it configurable? Then it remains the same field type, it just became configurable by install e-mail module.

Berdir’s picture

What I really want to avoid is making more modules required. We want to get rid of them, not add more. required API's shouldn't be in modules. Every required module slows down the whole test suite, because every single web test needs to install them, and every module installation is a costly container rebuild, plugin caches get reset and so on.

Other than that, not sure. Not convinced that replacing core plugins is a good idea, that seems like it could trigger unexpected behavior.

If it's just about moving a schema method into the Core classes and use the @FieldType annotation instead, that would be fine with me. Looking at email.module and ConfigurableEmailItem, the constraint stuff should be in the Core class anyway.

smiletrl’s picture

@yched
When I said, 'a simple piece of data', what I meant was , StringItem atm belongs to namespace 'Drupal\Core\Entity\Plugin\DataType'. So it's kind of like a 'data'...

But yeah, thanks for your so much clarification:)

fago’s picture

smiletrl’s picture

@fago, yeah, thanks for clarifying that:)

couldn't we just use a alter hook to swap out the plugin class and make it configurable?

This sounds a good plan. It will decouple dependency between configurable field ItemClass and base field ItemClass, and yield the unified field_type for similar fields.
While this solves @Berdir's 'required modules' issue greatly, it remains all these ItemClasses existing in code, kind of duplicating code as @yched unhappy with. Looks like this is a compromised solution for option1 and option2 + option3:)

yched’s picture

use a alter hook to swap out the plugin class and make it configurable

Dunno, sounds hackish and potentially confusing to me :-(.
Also, when we start to want to use widgets and formatters for those base fields, then repeat the dilemma for the widget / formatter classes - either the module that provides them becomes required (option 2), or they need to be in Core/Entity too (option 3).
In short, this hack doesn't buy us much IMO.

So, if requiring modules is too problematic,

Reevaluate option 3) ?
Put field type plugin classes (in their "configurable" shape) that are deemed basic enough in Core/Entity, along with their widgets and formatters.
Put their "moduley" bits (hooks, config settings, update hooks... routes ?) in entity.module
Drawbacks mentioned above:
- This is a weird split of code between Core/Entity and entity.module, but well...
- Folders becoming a soup of widget and formatter classes. I guess if classes are properly named, that's a minor drawback... Unless the annotations discovery can discover plugin classes nested in subfolders, but we can't really bet on it happening.
- Where does this put entity_ref ? file, image ?

Intermediate option 4) : ship those field types in a single "basic fields types" module ? (that one becomes required...)
Alleviates the "Core / module split" issue, still leaves the "folder organisation" & "where is the line" issues.

fago’s picture

- This is a weird split of code between Core/Entity and entity.module, but well...

Yes, I think this split is already weird: Please see #2031717: Make entity module not required. If it would go away, the stuff would even have to go the system module.

I think we should try to keep really essential base fields out of modules and just add the more complex ones with module(s). Having all in modules feels wrong as it renders the core component useless without modules.

Put their "moduley" bits (hooks, config settings, update hooks... routes ?) in entity.module

How much of that exists. What would be candidates for moving to the core component?

yched’s picture

Put their "moduley" bits (hooks, config settings, update hooks... routes ?) in entity.module

How much of that exists. What would be candidates for moving to the core component?

Well that's a good question ;-) See the many "where is the line?" above.

- I guess text field types should move to the core component (with their widgets / formatters)
Has a config/text.settings.yml, with associated text_update_8000()
Implements hook_library_info(), hook_filter_format_update(), hook_filter_format_disable()

- Number too - at least integer, but float and decimal use the same base class, widgets & formatters
No "moduley" bits

- email, telephone are absolutely trivial, they look a bit silly each in their separate modules right now
(the situation with email is particularly ridiculous, with
ConfigurableEmailItem extends Core\Entity\...\EmailItemEmailItem extends *LegacyConfigFieldItem* !)
Their only hook implementations are to "grab" the text formatters and widgets if text.module is enabled, becomes moot if those are provided in Core.

- Date would make very much sense, but would need some work, not sure for D8...
Has some heavy code to provide a FAPI #type element, would need to move to system
Perf might be a concern (the field type currently creates a DrupalDateTime object, and has to cache it in the field data cache for performance)

- List types & options widgets are more blurry, dunno if we have cases for those in base fields.
The bool field type most probably, but we'd need to untangle it from the others - we might try to push #2015689: Convert field type to typed data plugin for options module in that direction.
Implements hook_field_update_forbid()
Defines hook_options_list_alter()

- Link: not sure either ?

- Entity_ref is probably too big, I guess we'll keep ConfigurableEntityReferenceItem extends EntityReferenceItem ?
- File & Image are beasts too, and we probably don't have a case for base fields (at least not in current core entity types)
- Taxo_ref, well... :-)

All of the above have a config/[module].schema.yml (provides config schema for the bits of field.field*.yml and field.instance.*.yml that depend on the field type)

So, right, not *that* many moduley bits in a minimal scenario.

Also note that if we went with Core/Field as a subsystem in #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system, then those module bits could go in field.module rather than entity.module :-)

smiletrl’s picture

Regarding email LegacyConfigFieldItem, see #2087381-4: Decouple EmailItem field class from LegacyFieldItem

yched’s picture

Yes, the point in the last couple comments is that we should get rid of email.module and make the EmailFieldItem provided in Core be the one and only class used for both base fields and configurable fields.

That would mean shipping ConfigFieldItemInterface & ConfigFieldItemBase in Core too, but I don't see that as a problem.

andypost’s picture

@yched so is there any meta with roadmap to fix that? what we should do first and next?

Berdir’s picture

Status:Active» Postponed

I tried to merge e.g. the email class in #2023563: Convert entity field types to the field_type plugin but we need the FieldDefinition class that is being added in #1994140: Unify entity field access and Field API access.

Berdir’s picture

Issue tags:+Entity Field API

Tagging.

Berdir’s picture

The two issues got in.

The base field issue already came up with a pattern for now, that is non-configurable are altered to configurable with hook_field_info_alter(). I think we decided to re-visit that after #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system, when we can have configurable fields + widget/formatters in Drupal\Core.

Berdir’s picture

Issue summary:View changes

bla

Berdir’s picture

Issue summary:View changes
Status:Postponed» Active

Setting back to active.

If we decide that we want to move the email field completely into core, or the text field types, which are already in a required module, then we could try that now.

Or we can say we're ok with the current state and just close it.

yched’s picture

I'd be totally in favor of #24 :-)

yched’s picture

In fact, I'd be in favor of removing the distinction between "field types" and "configurable field types, i.e have all field types be "configurable":
- they all have to provide a schema() (this is needed to be able to have base fields stored in "automatic tables" anyway)
- they all have to provide settingsForm() and instanceSettingsForm() implementations (the base FieldItem class would provide empty ones)
- drop ConfigFieldItem / ConfigFieldItemInterface / ConfigFieldItemList / ConfigFieldItemListInterface (well, in order to be able to ConfigFieldItemList completely, we need #2047229: Make use of classes for entity field and data definitions and #2114707: Allow per-bundle overrides of field definitions to be solved - until then we still need ConfigFieldItemList's override of getFieldDefinition())

yched’s picture

Status:Active» Closed (duplicate)

Now that we know we need schemas on all field types, and are starting to leverage widgets/formatters on base fields, I feel it would be better to reboot that discussion in a fresh issue.

I opened #2150511: [meta] Deduplicate the set of available field types to start fresh and come up with an overview of our field types.
Closing this one.