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
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)
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 333 bytes | andypost |
#40 | interdiff.txt | 34.43 KB | andypost |
#38 | remove-email-module-2150511-38.patch | 8.65 KB | Berdir |
Comments
Comment #1
fagoI'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 :/
Comment #2
yched CreditAttribution: yched commentedWell, are there really strings that don't need sanitization ?
Comment #3
fagoSure 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?
Comment #4
fagoTo clarify, yes they need sanitization, but not processing - while processing includes sanitization it's not the same.
Comment #5
amateescu CreditAttribution: amateescu commentedI 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.
Comment #6
yched CreditAttribution: yched commented"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...
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedThis seems like something that would be pretty disruptive to contrib if done after beta, so let's try to get it in before then.
Comment #8
fagoImho, 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().
Comment #9
yched CreditAttribution: yched commentedI don't understand. TextProcessed does:
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 ?
Comment #10
yched CreditAttribution: yched commentedRegarding '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 ?
Comment #11
amateescu CreditAttribution: amateescu commentedSince 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'.
Comment #12
sunRegarding 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?
Comment #13
BerdirYes, 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.
Comment #14
andypostJust 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
Comment #15
yched CreditAttribution: yched commented@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.
Comment #16
andypostInitial stub patch, allows to use integer and string fields
Comment #17
andypostremove unused alter and apply to
node.title
Comment #21
andypostFix broken tests
Comment #22
andypostLet text inherit string item, also uuid has strange annotation
Comment #25
andypostThe constraint message should be renamed to use field name
Comment #27
andypostformatted summary with current patch state
Comment #28
tstoecklerRe-rolling this.
Comment #29
tstoecklerHere 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.
Comment #31
BerdirA lot in here will conflict with/no longer be necessary after #2198917: Use the string field type for the node title field.
Comment #32
tstoecklerOops, 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.
Comment #33
BerdirNo, 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.
Comment #34
tstoecklerOk, 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.
Comment #35
BerdirThat'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.
Comment #36
tstoecklerMakes sense. Making it official :-)
Comment #37
BerdirAnd back to active :)
Comment #38
BerdirThis removes email.module and moves the formatter and widget to Drupal\Core\Field and the test to field.module.
Comment #40
andypost1) fix tests for #38
2) move number field type to core
let's see how many tests are broken
Comment #41
andypostComment #43
andypoststandard profile breaks installer
Comment #45
andypostFix broken tests
Comment #46
Berdir> 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...
Comment #47
andypostFixed 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
branch2150511-base-fields
Comment #48
BerdirThis 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?
Comment #49
andypostFiled #2218199: Move email and number field types to \Drupal\Core\Field, remove modules
Comment #50
BerdirThanks, setting this to active then.
Comment #51
BerdirI 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.
Comment #52
YesCT CreditAttribution: YesCT commented#2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI adds a todo related to this. just noting.
Comment #53
BerdirI 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?)
Comment #54
Wim LeersI agree with Berdir, #52 is very confusing. Identical comment was posted at #2226493-105: Apply formatters and widgets to Node base fields.
Comment #55
yched CreditAttribution: yched commentedYeah, 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).
Comment #56
BerdirAs 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 :)
Comment #57
BerdirYeah, expected as much :) Is anyone opposed to doing that?
Comment #58
BerdirUnsurprisingly, DateItem is completely broken: #2239241: DateItem is completely broken, remove it. Will re-upload my patch there.
Comment #59
BerdirOk, 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.
Comment #60
sunSounds good to me. #2202925: Move simple field types into Core/Field should probably converted into a meta.
Comment #61
yched CreditAttribution: yched commented[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" ?
Comment #62
sunIf 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.
Comment #63
yched CreditAttribution: yched commentedYeah, 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.
Comment #64
Berdir1) 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.
Comment #65
yched CreditAttribution: yched commentedYeah, 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 ?
Comment #66
Wim Leers… and therefore the logical question:
— 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.+1
Regarding naming: "plain text" vs. "rich text" may be another option.
Comment #67
andypostIt'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
Comment #68
andypostBig +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)
Comment #69
BerdirThe 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.
Comment #70
yched CreditAttribution: yched commented@andypost #67 is out of scope for D8...
Comment #71
Wim Leers#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".
Comment #72
yched CreditAttribution: yched commentedWhat about field types machine names, then ?
Comment #73
BerdirOk, 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...
Comment #74
yched CreditAttribution: yched commentedThen 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' ??"
Comment #75
fagoI 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.
Comment #76
andypostI think date time needs own issue to move to Core/Field
Comment #77
andypostFiled #2331961: Move datetime module into Core/Field - maybe postpone this on #2318605: uuid, created, changed, language default value implementations need to be updated
Comment #78
fagoOpened related #2342581: Merge list field types into main field types.
Comment #79
joelpittetWhat'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.
Comment #80
YesCT CreditAttribution: YesCT commentedComment #81
andypostComment #83
xjmThis 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.