Updated: Comment #25

Problem/Motivation

There's a duplicated set of field types in core (see Original report) but core's "initial field types does not have widgets and formatters.

Proposed resolution

Move email and number modules code into \Drupal\Core\Field\Plugin\Field.
Inherit them for text and number module at least

Remaining tasks

figure out the core's default formatters|widgets

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions
Update the issue summary noting if allowed during the beta Instructions

User interface changes

tbd

API changes

no

Original report by @yched

Most base fields still use the "initial" set of field types that were introduced with the initial Entity/FieldNG patch.
Those have no widgets/formatters, and many of them duplicate the "configurable field types"

- Needless duplicates means confusion & memory clutter for field type plugin definitions
email / configurable email
string / text
timestamps
booleans...
- we are starting to leverage widgets/formatters on base fields

Several issues are touching this area:
- #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title)
- sub-issues opened from #2112239: Convert base field and property definitions
#2149845: Convert the description field of the 'aggregator_item' entity to a text field
#2149859: Convert the 'field_id' base field on comment entities to an entity reference field
- some other issue I can't find about user signature
- ...

We probably should:
1) take a broader overview of our current list of "base only" field types, and decide what to do with each of them.
2) then open sub issues per entity type and change the field types of their base fields accordingly (will probably affect the base table schemas)

Comments

fago’s picture

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 :/

yched’s picture

Well, are there really strings that don't need sanitization ?

fago’s picture

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?

fago’s picture

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

amateescu’s picture

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.

yched’s picture

"yes they need sanitization, but not processing - while processing includes sanitization"
Hmm... TextItem->processed is all about sanitization ? Through check_plain() or check_markup() depending on the field settings, but sanitization all the same. And user name / comment subject still need to run through check_plain, don't they ?
Not saying that we might not need a separate a separate field type, but I'm trying to clarify why exactly (because then it means separate widgets & formatters for the new field type).

Boolean: yes, it would really make sense to move the current "configurable" list_boolean out of the list_* stack and into a simple, standalone field type.

Timestamp: yeah, we'll most definitely want to allow "In place edit" of node.created/changed with datetime widgets...

effulgentsia’s picture

Issue tags:+beta target

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.

fago’s picture

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

yched’s picture

I don't understand. TextProcessed does:

if ($item->getFieldDefinition()->getSetting('text_processing')) {
  $this->processed = check_markup($text, $item->format, $item->getLangcode());
}
else {
  $this->processed = nl2br(check_plain($text));
}

This is the direct descendant of _text_sanitize() in D7, the job of "processed" has always been "prepare a safe-for-display string" AFAIK. Are you saying that the check_plain() part should move elsewhere because it doesn't fit the way we have named the class and property ?

yched’s picture

Regarding 'date':
The way I understand @Berdir's explanations in #2144327-30: Make all field types provide a schema() (basically, this is intended for dates as ISO strings, timestamps would be stored differently), I'd think we have no reason to keep a separate base-only 'date' field type and a configurable 'datetime' field type ?

amateescu’s picture

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

sun’s picture

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?

Berdir’s picture

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.

andypost’s picture

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

yched’s picture

@andypost : "data types" are off topic. This is about *field types* used by base fields, for which in most cases there are no existing widgets / formatters, since widgets / formatters historically were reserved to configurable fields, and most base fields still use dedicated field types.

andypost’s picture

Status:Active» Needs review
StatusFileSize
new6.86 KB
FAILED: [[SimpleTest]]: [MySQL] 63,393 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Initial stub patch, allows to use integer and string fields

andypost’s picture

StatusFileSize
new3.49 KB
new7.65 KB
FAILED: [[SimpleTest]]: [MySQL] 63,402 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

remove unused alter and apply to node.title

The last submitted patch, 16: 2190345-base-fields.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 17: 2150511-base-fields-17.patch, failed testing.

The last submitted patch, 17: 2150511-base-fields-17.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new8.81 KB
FAILED: [[SimpleTest]]: [MySQL] 63,407 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Fix broken tests

andypost’s picture

StatusFileSize
new4.97 KB
new13.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,121 pass(es), 430 fail(s), and 266 exception(s).
[ View ]

Let text inherit string item, also uuid has strange annotation

The last submitted patch, 21: 2150511-base-fields-19.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 22: 2150511-base-fields-22.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Related issues:+#2002168: Convert form validation of terms to entity validation
StatusFileSize
new3.16 KB
new13.16 KB
FAILED: [[SimpleTest]]: [MySQL] 51,698 pass(es), 487 fail(s), and 362 exception(s).
[ View ]

The constraint message should be renamed to use field name

Status:Needs review» Needs work

The last submitted patch, 25: 2150511-base-fields-25.patch, failed testing.

andypost’s picture

formatted summary with current patch state

tstoeckler’s picture

Assigned:Unassigned» tstoeckler

Re-rolling this.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new10.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 29: 2150511-29.patch, failed testing.

Berdir’s picture

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

tstoeckler’s picture

Assigned:tstoeckler» Unassigned

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.

Berdir’s picture

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.

tstoeckler’s picture

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.

Berdir’s picture

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.

tstoeckler’s picture

Status:Needs work» Postponed

Makes sense. Making it official :-)

Berdir’s picture

Status:Postponed» Active

And back to active :)

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new8.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,236 pass(es), 8 fail(s), and 1 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 38: remove-email-module-2150511-38.patch, failed testing.

andypost’s picture

StatusFileSize
new34.43 KB
new42.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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

let's see how many tests are broken

andypost’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 40: 2150511-base-fields-40.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new333 bytes
new42.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,184 pass(es), 19 fail(s), and 4 exception(s).
[ View ]

standard profile breaks installer

Status:Needs review» Needs work

The last submitted patch, 43: 2150511-base-fields-43.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new7.48 KB
new49.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,186 pass(es).
[ View ]

Fix broken tests

Berdir’s picture

Issue tags:+API change

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

andypost’s picture

Issue summary:View changes
StatusFileSize
new11.75 KB
new52.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,203 pass(es).
[ View ]

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

Berdir’s picture

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?

andypost’s picture

Title:Deduplicate the set of available field types» [meta] Deduplicate the set of available field types
Related issues:+#2218199: Move email and number field types to \Drupal\Core\Field, remove modules
Berdir’s picture

Status:Needs review» Active

Thanks, setting this to active then.

Berdir’s picture

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.

YesCT’s picture

Berdir’s picture

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

Wim Leers’s picture

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

yched’s picture

Yeah, DateTilme is an issue.

Not too thrilled either by the fact that we have string, string_long, text, text_long (and, well, text_with_summary).

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new1.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,298 pass(es).
[ View ]

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

Berdir’s picture

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

Berdir’s picture

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

Berdir’s picture

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.

sun’s picture

Status:Needs review» Fixed

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

yched’s picture

Status:Fixed» Active

[edit: Crosspost, reopening - actually, @sun, this one is for you]

Regarding string types (string, string_long, text, text_long, text_with_summary)

- string / string_long are only about storing raw strings, no "text format" - great
- text / text_long *can* optionally hold a text_format ('text_processing' setting, can be different from bundle to bundle, and can be modified in the field's lifecycle) . If this setting is FALSE, the field is functionnally strictly equivalent to string / string_long, it just has a different field type.
- text_long_with_summary is its own strange beast, present for legacy behavior of the body field and its "summary" UI from D5...

We have already established that we want to keep a basic field type without the overhead of (even optional) format handling. That's string*.

So I guess the question are :

1) Do we really want to have a field type where you can say "no, this field won't store formatted strings", and that allows you to change you mind later on and say "this will store formatted text after all, just treat the exising values as having the NULL format" ?
Or reversedly, "don't mind the current formats that were stored for the strings, treat them as plain, non formatted strings from now on".

@sun, I'd be interested to hear your thoughts on this :-)

2) Do we have a case for "short (varchar) string with a format" ?

sun’s picture

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.

yched’s picture

Yeah, I could have been clearer :-/

Question 1) is "I propose we have string / string_long be about raw strings and text / text_long be about (unconditionnally) formatted text, and you need to use migrate if you want to switch an existing field from one to the other".

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

Question 2) is "do we want to keep text[_not_long] ?" (= varchar with a text_format)

--> I take @sun's reply as a yes.

Berdir’s picture

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.

yched’s picture

Yeah, I guess we'd have to massage the UI names a bit. Maybe :
Unformatted string
Unformatted string (long)
Formatted text
Formatted text (long)
Formatted text (long, with summary)

Or maybe use "[Un]Formatted text" for both ?

Wim Leers’s picture

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

andypost’s picture

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

andypost’s picture

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)

Berdir’s picture

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.

yched’s picture

@andypost #67 is out of scope for D8...

Wim Leers’s picture

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

yched’s picture

What about field types machine names, then ?

Berdir’s picture

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

yched’s picture

Then we have the "dates" area :

- Core's 'timestamp' / 'created' / 'changed' field types, storing UNIX timestamps
Currently not available in Field UI's "add new field", thus intended for base fields only
- datetime.module's 'datetime' (UI label "Date"), storing ISO 8601 date strings with optional times (and timezones ?)
The only "date" field available for custom fields for site admins.

It does looks like we need two separate field types for UNIX timestamps and ISO 8601 dates.

I don't see why we have the 'created' and 'changed' field types though. They basically are just TimestampItem with :
- 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 ?
- for 'created': a specific default value of REQUEST_TIME. Looks like there should be a way to specify this in the BaseFieldDefinition of a timestamp field ? Maybe #2318605: uuid, created, changed, language default value implementations need to be updated should allow that ?

Also, the field type names are weird :-p : "A field of type 'changed' / of type 'created' ??"

fago’s picture

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

andypost’s picture

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

andypost’s picture

fago’s picture

joelpittet’s picture

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.

YesCT’s picture

Issue summary:View changes
Issue tags:+Needs issue summary update