[meta] Deduplicate the set of available field types

I'm wondering whether there are any downsides to moving the configurable variants, which e.g. might have more options.

I think generally it should be fine, but there is on thing that worries me a bit:
Compared to the simple string field, the text-field is more complex and has additional properties (format, processed) as it optionally does text processing also. We might want to make format,processed properties conditionally available to avoid e.g. lots of processed property objects being instantiated always. However, this seems to be much cleaner done in a separate class instead of having one complex class with lots of conditionals everywhere - but that would mean a separate type again :/

Sure they need a check_plain, but so far $user->name or $comment->subject do neither have a format nor a processed property. Why would they, if they aren't processed?

To clarify, yes they need sanitization, but not processing - while processing includes sanitization it's not the same.

I agree with fago, the string item still has its use cases.

In the end, I think we want TextItemBase to extend StringItem when ConfigFieldItemBase will get out the way.

The email and boolean fields should definitely merge into Core\Field, but date/timestamps.. that won't be fun to figure out.

This seems like something that would be pretty disruptive to contrib if done after beta, so let's try to get it in before then.

Hmm... TextItem->processed is all about sanitization ?

Imho, it's all about processing. If you enable the text processing option, your text will be processed with the help of input formats. The processed output has to be sanitized yes, but still imo the reason for having the property is that we might need to do some processing that's more than a simple check_plain().

Since most (all?) core entities will use the 'timestamp' and 'changed' field types as base fields, I agree that 'date' should be removed in favor of the configurable 'datetime'.

Regarding String vs. (processed) Text, the latter has a large amount of dependencies (→ Filter module → text format configuration → module system → filter plugins) and has a bold design aspect of only caring for an HTML output format baked-in.

Compared to that, String does not appear to have any dependencies at all and also doesn't assume HTML output?

As someone unfamiliar with the system (but familiar with Drupal [7]), that separation did make sense to me.

Of course, there's the question of whether Text could at least extend String, but given that Text's value cannot or should be used by itself in almost all cases, I wonder whether that extension wouldn't make typical instanceof String conditions harder, without providing a concrete benefit?

Yes, as already commented in the node title issue, I think that text is unnecessary overhead for most existing strings and should only be used if it can have a text format.

Just faced with that, this should really help contrib to make build their entity forms by definition.
Also this will help #1875974: Abstract 'component type' specific code out of EntityDisplay to simplify search for fields and available widgets/formatters

This really needs to investigate the map of dataType and fieldType plugins.

The feature/api change could be optional schema definition for dataTypes or some kind of mapping for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions

Initial stub patch, allows to use integer and string fields

remove unused alter and apply to node.title

Fix broken tests

Let text inherit string item, also uuid has strange annotation

The constraint message should be renamed to use field name

formatted summary with current patch state

Re-rolling this.

Here we go. I took the liberty of changing the code style of a couple hunks and I left one hunk out that I did not understand.

A lot in here will conflict with/no longer be necessary after #2198917: Use the string field type for the node title field.

Oops, wanted to unassign after the re-roll.

Re #31: Hmm.. I wasn't aware of that issue. Seems this should be closed as duplicate, but I'll let @andypost make the call.

No, it's not a duplicate, that's not what I mean, sorry if I was unclear.

The other issue is only about the string field type and widgets for that. Which is what currently makes up most of this patch but this should eventually also cover a lot of other field types. Maybe in separate issues, maybe as a single patch, I don't know yet.

I just meant to let this one sit until the other is in and then continue from there.

Ok, right. The issue title and the current patch don't really match. But I think the current *patch* at least is - unless I'm mistaken - entirely duplicated by that issue.

That's correct, the only thing that might still make sense is extending TextItemBase from StringItem but what we can gain from that is minimal (removal of the constraint method).

Let's wait for #2192259: Remove ConfigField.. Item and List classes + interfaces to land and then we can continue here. That should make it easier, as the tricks with ConfigFieldItemInterface and related methods can just go away.

Makes sense. Making it official :-)

And back to active :)

This removes email.module and moves the formatter and widget to Drupal\Core\Field and the test to field.module.

1) fix tests for #38
2) move number field type to core

let's see how many tests are broken

standard profile breaks installer

Fix broken tests

> abstract class DefaultNumberFormatter extends FormatterBase {

Should we rename this to something like NumberFormatterBase? that would be more in line with others and we move it anyway.

> NumberDecimalFormatter
> NumberIntegerFormatter

Do we still need the Number prefix for the class name and id here? Just DecimalFormatter and IntegerFormatter would be more in line with others I think? MailToFormatter also uses mail_mailto, should that just be mailto for the id? I'm not sure... Would require bigger changes to tests and break existing configurations, if we want to do it, better do it now?

number_unformatted is a bit different I think, there we need a prefix... so we could just as well leave them? Although as a class name, UnformatterNumberFormatter would read better?

> * Defines the 'decimal' entity field type.

Let's just use "field type", we're trying to get away from the term "entity field type" I think. Don't worry about the unchanged field type classes :)

The diff stat looks great here. I'm not sure how far we want to go and if we should split up, though? That's what the issue summary is suggesting too, although it's a bit old.

BooleanItem and ListBooleanItem is another overlap, but that also relies on more stuff and is heavily touched already in #2169983: Move type-specific logic from ListItemBase to the appropriate subclass, maybe that issue could merge it, or a follow-up of that?

That leaves EntityReferenceItem and DateItem, both those two modules are a lot bigger than the others. Not sure about what to do with them...

Fixed all suggestions, but used NumericItemBase, NumericFormatterBase, NumericUnformattedFormatter - imo it makes more sense.

Also fixed config schema (removed prefix) and code comments that point to prefixed field types.

Let's decide on prefixes for formatters!
I think we need to preserve prefix for formatter IDs (just 30 hunks to fix),
... maybe it make sense to change prefix to 'numeric_' but prefer to leave as is!

I'm ok to address Boolean, Date and ER in separate issues

EDIT pushed to sandbox/yched/1736366 branch 2150511-base-fields

This looks great.

Let's change this to a meta issue so that we can this in in a new issue and out of the way?

Filed #2218199: Move email and number field types to \Drupal\Core\Field, remove modules

Thanks, setting this to active then.

I think this is close to done with #2226063: Merge ListBooleanItem from options module into BooleanItem?

We still have some field types that we could move to Drupal\Core\Field but we have #2202925: Move simple field types into Core/Field for that.

The issue summary mentiones timestamps but there are no timestamp field types in modules. What we do have is DateItem that is unused and AFAIK broken. Can't move datetime easily to core as that depends on a ton of render elements and theme functions.

#2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI adds a todo related to this. just noting.

I don't think that @todo is correct, this is issue is pretty much done as mentioned above. I think @pwolanin was working on that part (enabled boolean widget, right?)

I agree with Berdir, #52 is very confusing. Identical comment was posted at #2226493-105: Apply formatters and widgets to Node base fields.

As mentioned, maybe we should just kill DateItem? Posting as a suggestion, can make a separate issue if we think that's OK. No idea if we have any test coverage for this.

About the string/text, yeah, do you have any suggestions what we could do there? Because I don't. We do have valid use cases in core for string_long fields, for example. I'd argue that while we have too many slightly different types, they're not really duplicates :)

Yeah, expected as much :) Is anyone opposed to doing that?

Unsurprisingly, DateItem is completely broken: #2239241: DateItem is completely broken, remove it. Will re-upload my patch there.

Ok, DateItem is gone :)

Unless you have ideas on how to unify string/text field types, I suggest we close those issue.

If we want to move more field types to core and kill some dead-simple modules like telephone, I suggest we do that in #2202925: Move simple field types into Core/Field as it is no longer about deduplicating.

Sounds good to me. #2202925: Move simple field types into Core/Field should probably converted into a meta.

If I interpreted you correctly, then the question is: Should the format really be optional for text* fields?

I think it should be required. IIRC, it is only optional right now, because there were no string* fields originally, so we lumped "that other use-case" into text* fields. It should not be possible to remove a format from a text* field. (If it's possible to disable/remove the format after creating data right now, then that's a security issue.)

re: 2) Isn't that what the 'text' (sans _long) is? (A varchar with a format.) If the question is whether we should remove it; I'd rather keep it. The original idea was to allow e.g. title fields to have a format, so that you could use input filters in there (think of e.g. Markdown). Further use-cases can be found easily.

1) Hm. Agree that it would technically be possible to remove that option, results in less complexity in there. Downside is that we give the user more options, we would then remove the no_ui from StringItem and StringLongItem and show them too, so the user would have 4-5 option to chose from instead of 3 and then format yes/no. We would have to make that very clear... not sure how technical "String" as a term is, so maybe it would have to be "Text" and "Text with Format" or something like that?

The main reason for changing it is that you initially forget to set it, like creating a field, want to create content and then you notice it, go back, enable/change it and then you don't touch it again. This would no longer be possible, but it would also happen less often if you need to decide between them on the first step.

2) Yes, I think there are valid use cases for that, as sun mentioned. Not sure if string_long without text format needs to be shown, though.

- text_long_with_summary is its own strange beast, present for legacy behavior of the body field and its "summary" UI from D5...

… and therefore the logical question: Can we please remove this beast? — that's now possible because people must migrate their content anyway: the otherwise broken upgrade path is no longer a blocker. We could finally push people towards the correct way of doing this, which is to have a separate "summary" field.

If I interpreted you correctly, then the question is: Should the format really be optional for text* fields?

--> I take @sun's reply as an approval.

+1

Regarding naming: "plain text" vs. "rich text" may be another option.

It's really nice to see UX concerns raised here.
Sometime ago webchick filed a screenshot asking about a quantity of field types and a hard way to properly choose which one to use, how we would learn people to properly use them.

Looking on 5 text field types + email and phone I think they could be "deduplicated" somehow.

Maybe it's time to introduce a kind of "field behavior" like ER has:
1) format, placeholder, validation - could be attached as behavior. But once field created and holds data there should be no way to change that except migration
2) otoh we could allow to change that by storing more information in field table so formatter/widget would be chosen depending on actual data stored. The new entity form will use widget that currently stored in field (instance)

1st seems easy to implement but 2nd is more like document-driven and could be targeted for contrib/9.x

Can we please remove this beast?

Big +1 here, currently there's no way to ship own node type with/without body in module/profile because body field is always added (as I remember once you try to put body field into profile you will got exception)

The options of the existing setting are 'Plain text' vs. 'Filtered text'. Filtered makes more sense to me than Rich, but I do like Plain instead of unformatted.

@andypost: Whether 5 clear options upfront or one and then options that you do not know yet and might overlook later is better is arguably. The body problem is a problem, but it's a different one, I will open an issue for this, not related to this issue.

#69: "Filtered" is definitely more accurate. But it is a Drupalism, which is why I proposed "Rich". However, considering this is the Drupal admin/site building UI, I think it's better to err on the side of precision rather than on the side of "more widely understood without training". So +1 to "filtered".

Ok, started #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI. Not touching the summary field type, that's a different topic and also didn't rename any machine names yet, that would need a lot more changes...

- for 'changed': a preSave() method that assigns REQUEST_TIME -> This seems like it should be the job of the entity type to assign this behavior to one of its timestamp fields if it sees fit in its own preSave(), why would that deserve a dedicated field type ?

I think a the changed field type is great to have, as it encapsulates the preSave() logic into the field class instead of having it flying around in X entity type classes wherever it's being used. I do think it's a great to have that logic re-usable and self-contained in the field type class and I don't think there is a problem in having such a non-UI field type?
I agree though that the naming isn't ideal, e.g. "date_changed" would sound already more logical our timestamp_changed to stress it's a timestamp.

@created, yeah when we add support for NOW as default value I agree that the field type is unneeded.

I think date time needs own issue to move to Core/Field

Filed #2331961: Move datetime module into Core/Field - maybe postpone this on #2318605: uuid, created, changed, language default value implementations need to be updated

Opened related #2342581: Merge list field types into main field types.

What's the necessity for "plain" vs "formatted" text distinction for data storage on the "text" field type?

It feels as though, when we have a second thought on "text" type and want to open it up to "formatted" so I can get a WYSWYG editor in D7 it was a matter of changing the text format, in D8 I now have to likely create a new field and migrate it or hack the database it seems from the outset.

Reason I'm bringing this up is in D7 I had a similar problem with "Text with Summary" because once it's set it can be tricky to change and usually just "hide" the summary as the solution for body field and all others try not to use that field type in the first place...

The added granularity in the site builder UI that is not easily to change (even if it does mean data loss + a warning) seems to just add to confusion. Sorry if this sounds ranty.

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.