Remove text_processing option from text fields, expose existing string field types as plain text in UI

Thanks a ton for pushing this, @Berdir.

Nitpick : I think "Filtered long text" is a bit weird. If I'm not mistaken, in proper english you'd tend to say "Long filtered text", "filtered" being a more intrinsic characteristic than "long".

Given that Number and List field types already use parenthesis to be alpha-sort friendly, I'd think we can go with :
Filtered text
Filtered text (long)
Filtered text (long, with summary)
Plain text
Plain text (long)
?

Other than that, the possibility of adding optgroups in that list was mentioned a couple times, we just never actually crossed that line. Would require adjusting the annotations a bit, so probably for a separate issue. Just mentioning the possibility in case we find no way satisfying way to structure the flat list.

Side note : #2226493: Apply formatters and widgets to Node base fields already adds 'string_long' field type to the TextTextarea widget.

Looks great, from an editor.module POV!

I only made one functional change: PlainTextEditor should not limit itself to the "string" fields, it should only limit itself to not support "filtered text" fields. It's more likely that special-purpose string-like fields will be added than that more filtered text-like fields will be added. Hence the inverse condition makes more sense :)

The remaining changes that I made are all to reflect that we are now effectively officially calling "processed text" fields "filtered text" fields. Hence I've updated editor.module and its plugins to reflect that. Consistent language helps with maintainability & understandability.

I did not try to fix failing tests in this reroll.

- *   label = @Translation("Filtered ong text and summary"),
+ *   label = @Translation("Filtered long text and summary"),

:D

Also fixed the Quick Edit test failure and one other.

Awesome, thanks for working on this.

Though, hm, can we think of a way to keep the options sorted/grouped together, using a shared prefix? They're all about text, so it's a bit confusing UX to have some options in one spot and some others elsewhere.

For example - focusing on the intended end result, so intentionally omitting text_filtered_long_with_summary:

Number (integer)
Text (filtered, long)
Text (filtered)
Text (plain, long)
Text (plain)
Term Reference

Looks much more pleasant to me?

Quite possibly separate issue, but we should also rethink "long". That's not very intuitive. It would make more sense to use "single-line" and "multiline", since both words are common language. (It doesn't matter that you technically can insert newlines/<br>s.)

Also about UX - there's #1963340: Change field UI so that adding a field is a separate task
Suppose this change would allow to provide a description for each field type to properly describe each field purpose

Quick review before I go afk for a couple days - not much to complain about :-)

1. TextfieldWidget now assumes it always deals with filtered text and unconditionally generate a #type = 'text_format' element, yet its annotation says it applies to 'string' field type ?

   Also, 'string*' field types have the following widgets :
   'string' = textfield
   'string_textarea' = textarea

   For 'text*' field types it's :
   'text_textfield' = textfield
   'text_textarea' = textarea

   Maybe the 'string' widget should become 'string_textfield' for consistency ?

2. +++ b/core/modules/text/src/Tests/Formatter/TextFormatterTest.php
   @@ -66,24 +66,6 @@ public function setUp() {
   -      'settings' => array(
   -        'text_processing' => TRUE,
   -      ),
   -    ))->save();
   -    entity_create('field_storage_config', array(
   -      'name' => 'unprocessed_text',
   -      'entity_type' => $this->entityType,
   -      'type' => 'text',
   -      'settings' => array(),
   -    ))->save();
   -    entity_create('field_instance_config', array(
   -      'entity_type' => $this->entityType,
   -      'bundle' => $this->bundle,
   -      'field_name' => 'unprocessed_text',
   -      'label' => 'Unprocessed text',
   -      'settings' => array(
   -        'text_processing' => FALSE,
   -      ),
   

   The comment above this code block ("Set up two fields: one with text processing enabled, the other disabled") is now stale ?

3. About TextPlainUnitTest : woah, this class looks confusing indeed. Lots of code for not much test, and the one actual test method claims it tests the 'text_plain' formatter, which no longer exists.
   At any rate, it looks like this test is now about the 'string' formatter for 'string*' field types, and no longer belongs to text.module ?

   It looks like it certainly could be skimmed down (wasn't there a commit that added assertStuffAboutContent() methods to DUTB, or did I dream it ?), but the "Verify that all HTML is escaped and newlines are retained" part would make sense to keep for the 'string' formatter - I vaguely remember nasty cases in that area, the regression check would be nice to keep.

The patch needs a reroll, so I tried that, I hope this isn't totally wrong. On top of that here are the adaptions to the test, so that the
previous functionality is still tested.

Verified that with this patch applied, we have zero remaining occurrences of "text_processing" (except in migration tests' simulation of D6/D7 databases, of course).

1. +++ b/core/modules/rdf/src/Tests/Field/TextFieldRdfaTest.php
   @@ -62,13 +65,13 @@ public function setUp() {
   -    // Tests the plain formatter.
   -    $this->assertFormatterRdfa(array('type'=>'string'), 'http://schema.org/text', array('value' => $this->testValue));
   

   Nitpick: Why do we want to remove this assertion? I think it's because we can no longer format filtered text fields as plain text. But shouldn't we then also create a StringFieldRdfaTest to ensure we don't reduce test coverage?

2. +++ b/core/modules/system/src/Tests/Entity/EntityViewControllerTest.php
   @@ -46,7 +46,7 @@ function setUp() {
   -  function testEntityViewController() {
   +  function dtestEntityViewController() {
   

   … we may want to re-enable this one :)

3. +++ b/core/modules/text/config/schema/text.schema.yml
   @@ -46,11 +44,9 @@ field.text_long.settings:
   +  label: 'Filtered long text settings'
   

   Shouldn't we also turn this into Text (filtered, long) settings'?

   Analogously for the other ones.

4. +++ b/core/modules/text/src/Tests/Formatter/TextFormatterTest.php
   index 167bcec..0000000
   --- a/core/modules/text/src/Tests/Formatter/TextPlainUnitTest.php
   
   --- a/core/modules/text/src/Tests/Formatter/TextPlainUnitTest.php
   +++ /dev/null
   

   Why can we delete this test?

I think this one is very close!

TextPlainUnitTest: cool :-)

Not sure whether #15 1. and 2. were adressed ?

General, big +1 on resolving the mess around current string + text field types. I'd already explain it that way that text fields are about text processing and their option to turn it off does not make sense given we have got the string field types to developers.
For sitebuilders this patch removes the ability to switch between text-processed fields on the fly, but I think that's ok and makes sense as it is/should be a change in the data model (there is no point in creating an unused format column). Field types should be perceived as "what data is going to be stored", what this patch encourages.

I've discussed with berdir a bit on IRC whether it makes sense to have a field type with machine name "string" and human readable label "Text (plain)", but given the different target audiences (devs / sitebuilders) I'd consider it ok in that case.

Finally, this patch starts to use the wording "filtered" instead of "processed", what makes sense given it's making use of the filter system ;). We could consier renaming the computed processed property to "filtered", but that would have to happen before beta also.

1. +++ b/core/modules/field/src/Tests/String/StringFormatterTest.php
   @@ -0,0 +1,128 @@
   +    $this->renderEntityFields($entity, $this->display);
   +    $this->assertNoRaw($value);
   +    $this->assertRaw(nl2br(String::checkPlain($value)));
   

   Moved around code, but this seems to to test $value only, but not the rendered output?

2. +++ b/core/modules/rdf/src/Tests/Field/StringFieldRdfaTest.php
   @@ -54,21 +47,14 @@ protected function setUp() {
   +    ¶
   

   Some whitespace here.

3. +++ b/core/modules/text/config/schema/text.schema.yml
   @@ -46,11 +44,9 @@ field.text_long.settings:
   +  sequence:
   +    - type: string
   

   Weird that this becomes a sequences now, it should be still a mapping even if empty, right?

   E.g. this is what the boolean field types uses:
   type: mapping
   mapping: { }

   Btw, why do we have field config for core field types below the field.modules?

Yeah, agreed that the field type names are not ideal yet - meaning, still tainted by history rather than reflecting the actual model.

Patch does:
string* = "Text" - with SQL underlying type TEXT, btw (well, or VARCHAR...)
text* = "Filtered Text"

That's not what we would have chosen if we were designing a fresh system :-) - more probably something like:
text
text_long
filtered_text
filtered_text_long
filtered_text_with_summary

But then it's another case of "A becomes C, B becomes A" nasty rename ('text' / 'text_long' become something different).
Also, it raises the question of "text.module provides filtered_text* but not text*" - which also relates to "do text.module, number.module still have a justification as modules" ?

All in all, I think there's room for enhancement there, but IMO it's reasonable to keep the renames for a separate issue, and tackle the funtional changes & field type remapping here.

Btw, why do we have field config for core field types below the field.module ?

Was wondering the same :-)
Since it's already the case in HEAD, it's fair for this patch to stay consistent for now. Opened #2325999: Move config schema for Core field types out of field.module

1. +++ b/core/modules/text/src/Tests/Formatter/TextFormatterTest.php
   @@ -54,7 +54,6 @@ protected function setUp() {
        entity_create('field_storage_config', array(
          'name' => 'processed_text',
   

   'filtered_text' ?

   + the class also has a "Verify the processed text field formatter's render array" comment that should be updated.

2. +++ b/core/modules/quickedit/src/Tests/EditorSelectionTest.php
   @@ -50,57 +50,42 @@ protected function getSelectedEditor($entity_id, $field_name, $view_mode = 'defa
      /**
   -   * Tests a textual field, without/with text processing, with cardinality 1 and
   +   * Tests a textual field, without/with text filtering, with cardinality 1 and
       * >1, always without a WYSIWYG editor present.
       */
      public function testText() {
   

   Comment needs to be updated, this now only tests "without" text filtering - and we lost test coverage for filtered text ?

3. +++ b/core/modules/rdf/src/Tests/Field/StringFieldRdfaTest.php
   @@ -54,21 +47,14 @@ protected function setUp() {
   +    ¶
   

   Whitespace

4. +++ b/core/modules/rdf/src/Tests/Field/TextFieldRdfaTest.php
   @@ -62,13 +65,13 @@ protected function setUp() {
   +    $formatted_value = strip_tags($this->entity->{$this->fieldName}->processed);
   

   $filtered_value ?

5. +++ b/core/modules/quickedit/src/Plugin/InPlaceEditor/PlainTextEditor.php
   @@ -32,8 +29,8 @@ public function isCompatible(FieldItemListInterface $items) {
   +    elseif (in_array($field_definition->getType(), array('text', 'text_long', 'text_with_summary'))) {
   

   4 places within editor/quickedit use that list, it should probably be centralized to a helper method somewhere (but not sure where :-/)

6. Agreed that TextProcessed.php should be renamed to TextFiltered, and the corresponding properties to ->filtered / ->summary_filtered. We might as well do it here ? (but I can live with a followup)

Also, very sorry for what I'm about to post, but thinking about it more I'm not sure "filtered text" is the right terminology :-/.

Being given a choice between "Text" and "Filtered text", the second option sounds more restricted.
The actual difference for users is: the former will give me a dumb text input where HTML won't be active, while the latter will give me a CKeditor where I can do lots of pretty stuff.

So I'd think "plain text / rich text" is more telling.

A "filtered text" only requires filtering because it's "rich" (and thus potentially dangerous), but that's a technical consequence, not the actual feature that makes sense to end-users, the actual feature is that it's "rich".

+ "filtered text / filters" are drupalisms, while everyone knows what is a "rich text editor".

I still think it's more accurate to talk about the "filtered version of a rich text" than of the "processed version" (in the cases where code / comments need to differentiate between the raw and safe versions), so the general "s/processed/filtered" move in this patch is still fine IMO.

Also, going with "filtered text" for the field type causes the ambiguity, present in a couple places in the patch, that you have code and comments about "the unfiltrered (raw) value of a filtered text" and "the filtered value of a filtered text". - i.e. the "filtered text" is not always actually filtered :-)

Talking about a "rich text" field lets you unambiguously talk about its raw and filtered values.

Not sure I'm able to follow - the screenshot in #13 had "Text (plain*)" vs. "Text (filtered*)":

It might make sense to rename "filtered" to "formatted", because users are more likely to understand the difference between plain text and formatted text, but that's just a minor UI/wording tweak.

(Renaming the internal IDs that way would not be sensible, since "formatted" would be ambiguous with Field Formatters, and "filtered" is pretty precise from a technical perspective.)

@sun: what I'm saying is that
- yes, technically, "filtered" is more accurate than "formatted", so we're right to move to the former where we're talking about "the string once passed through check_markup()"
- but either "formatted" or "filttered" make little sense to end-users, what makes sense for them is "rich" :-)

@yched: I don't agree. "rich" is an overloaded term due to historical IT evolution. It primarily assumes a familiarity with Microsoft products, which is (thankfully) no longer a given in today's world of consumer devices. Without that legacy association, "rich" doesn't encompass any sensible meaning to a human. A content may be "rich" of words (as opposed to e.g. poorly written), but there is no relationship to text editing, processing, or formatting. Contrary to that, the act of "formatting a text" is unambiguous and clear, even beyond the scope of computers.

@sun: you think "plain text / rich text" is ambiguous ?
RTE / "rich text editor" is common lingo, so I'd think people are much more akin to figure out what "rich text" means as opposed to "filtered text", which, to me, feels like a "less powerful version of a regular/plain text".

Contrary to that, the act of "formatting a text" is unambiguous and clear, even beyond the scope of computers

Maybe so, but "formatted" has been ruled out and is being removed from the UI.

Anyway, I don't intend to bikeshed on names here if this is controversial.
Fine with RTBC when pending remarks are adressed :-)

Will probably need to be adjusted after #2325999: Move config schema for Core field types out of field.module .

the filter system sometimes also uses processed/process/processing, see FilterProcessResult for example. In that way, this also seems a possible way out of the "(un)filtered text of a filtered text field", we could have a filtered text field (text + filter) that is processed or not?

Still feels messy if you ask me.. It seems a bit far-fetched to have "filtered" not mean "has been filtered", and "processed" mean "filtered text has been filtered" (even though I love "grumpy cat is grumpy" :-p)

Anyway, yeah, let's keep that for another issue.

Am I understanding it correctly that we're mostly talking about the UI labels and comments?

Well, in my mind that spills to the field type IDs and class names if we want to keep our sanity, but yeah - followup.

- re - #37-5 and hardcoded list of field types :
Agreed that hardcoding the list is not ideal, but that's what the patch does currently, so hardcoding in one place is a little better than hardcoding in four places ?
Then, we can talk with @Wim about how to change the content of the method from a hardcoded list to something extensible (an entry in the field type annotation ? a setting ?)

RE: #37.5: yes, let's discuss that in a follow-up. I just created that follow-up and already added all relevant information, and the general steps that should be taken: #2327941: Simplify in-place editor selection (Was: Refactor PlainTextEditor/WysiwygEditor/Editor in-place editors now that text fields no longer have configurable filteredness). In short: it will actually allow for a clean-up, and the code cited in #37.5 is necessary for now, but we should be able to remove that altogether. Fixing it here would lead us in the scope of Quick Edit module though, so let's do it in that follow-up I created.

Looks ready to me — just to nits:

1. +++ b/core/modules/editor/editor.module
   @@ -450,32 +451,32 @@ function _editor_delete_file_usage(array $uuids, EntityInterface $entity, $count
   +    return in_array($definition->getType(), array('text', 'text_long', 'text_with_summary'));
   
   +++ b/core/modules/editor/src/Plugin/InPlaceEditor/Editor.php
   @@ -32,10 +32,10 @@ public function isCompatible(FieldItemListInterface $items) {
   +    elseif (in_array($field_definition->getType(), array('text', 'text_long', 'text_with_summary'))) {
   
   +++ b/core/modules/quickedit/src/Plugin/InPlaceEditor/PlainTextEditor.php
   @@ -32,8 +29,8 @@ public function isCompatible(FieldItemListInterface $items) {
   +    elseif (in_array($field_definition->getType(), array('text', 'text_long', 'text_with_summary'))) {
   
   +++ b/core/modules/quickedit/tests/modules/src/Plugin/InPlaceEditor/WysiwygEditor.php
   @@ -30,10 +30,10 @@ public function isCompatible(FieldItemListInterface $items) {
   +    elseif (in_array($field_definition->getType(), array('text', 'text_long', 'text_with_summary'))) {
   

   Rule of thumb: Always use the strict comparison flag for in_array(), unless you explicitly want to perform a loose comparison; i.e., in_array(…, …, TRUE)

2. +++ b/core/modules/field/src/Tests/String/StringFormatterTest.php
   @@ -0,0 +1,128 @@
   + * @group text
   

   We can keep 'text' as a secondary @group, but the first @group should be 'field', since this test is in field.module.

312 insertions, 811 deletions.

A wonderful simplification.

Thanks, Berdir!

I think this needs change record and final update to summary

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -218,7 +218,7 @@ public function setCommentSubject($enabled) {
-        'type' => 'string',
+          'type' => 'string_textfield',

wrong indent, could be fixed at commit/re-roll

You're right. Back to NW for a change record.

After that, this can go straight to RTBC, #56 can indeed be fixed on commit (good catch though :)).

Looks great. RTBC +1.

This is perhaps a followup: since we are fixing this mess, what's the difference between text field and text field long? *I* know it's varchar vs text -- but shouldn't that be a field setting that the storage can decide upon? Would decrease the confusion here.

Also, nitpick:

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -218,7 +218,7 @@ public function setCommentSubject($enabled) {
-        'type' => 'string',
+          'type' => 'string_textfield',

Indent error.

re #60 - "short / long" as separate field types or a storage setting :
Can't really remember the exact reason why we made the split in D7, IIRC we tried to reduce the occasions were changing a setting would mean SQL type conversion. It would probably work, as we can forbid changing that setting once the field has data anyway.

Main drawback I see atm is that the default widget is typically not the same - textfield or textarea. So for example after adding a "text" field with "long" setting, you'd get a silly mono-line textfield widget until you to visit "Manage Form" to switch it "textarea".
Unless we refactor to a single widget outputs a textfield or textarea depending on the field...

At any rate, yeah, separate issue.

RTBC +1 on this one :-)

*I* know it's varchar vs text -- but shouldn't that be a field setting that the storage can decide upon? Would decrease the confusion here.

yeah, that has been bugging me as well in particular as we've the setting for integers. However, as one models the data structure using field types it seems to make sense to have a different field type for a different storage type, so there is a clear mapping. But then, that's a quite SQL centric view... Howsoever, separate issue.

Main drawback I see atm is that the default widget is typically not the same - textfield or textarea. So for example after adding a "text" field with "long" setting, you'd get a silly mono-line textfield widget until you to visit "Manage Form" to switch it "textarea".
Unless we refactor to a single widget outputs a textfield or textarea depending on the field...

I guess it would be nice to have the default be returned from a method, so it could take field settings into account.

Also sorry to bring this up but I agree with yched in #38 - the Text (filtered) looks very odd to me, and it doesn't match with either text editors or 'text formats' which is what we expose elsewhere. You only see filters for the configuration of filter plugins for text formats, and you can use Drupal for a decent amount of time without visiting that screen.

I think its rather odd that you have to make this choice upfront. From a strategic standpoint, I dont think we should be adding more and more to this list - but instead make the next step "field configuration" more important and better able at providing you the choices. chx gives a perfect example for this.

With that in mind, I think we should at least open a meta or followup for this. Because in the larger scheme of things it just doesn't make sense and won't scale. \

Other than that catch is right that the labeling isn't very usable. Can we perhaps just mention the actual concept like "text formats"? Like (with text formatting), we probably don't have to signal that the other one doesn't have it.

There was #1963340: Change field UI so that adding a field is a separate task but usability here also questionable

Just to clarify reasoning:

- "Formatted" might not be perfect, but it matches the language we use with "Text formats". If I have no idea what it means, I can at least scan the configuration page and the word format is in the menu pointing to the right place. Filtering as a concept you only see when you're actually configuring a text format (which is a very rare operation).

- "Rich text" this works as colloquial term to go along with rich text editor, which also gets you back to text formats if you follow that route (eventually). Also pretty clear what it means compared to "plain text".

Hmm, I hope we can just make a follow up to do this in a separate page. Then you can offer "text" and than provide two additional dropdown, that is much more sensible, than this combined in one dropdown approach. I feel that is the right way to go, and although I am sure this labeling is more consistent - at the end of the day we evaluate by how "usable" and it doesn't really meet that standard.

@Bojhan: The whole purpose of this issue is to split "plain text" and "formatted/rich text" into separate field types.

The rationale being:
- We need to keep a simple, lean and performant field type for base fields like "node title", "comment subject", ... that store raw strings. That is the 'string' field type in current HEAD.
- In HEAD, that 'string' field type is not available in the UI. Instead, there is the 'text' field type, which can, on a second screen, be configured to be about raw text or formatted text (= a text + a format).
- In the "raw text" mode, this field type thus behaves exactly like 'string', which means code duplication and DX confusion (not in the UI, but in the set of field types available to developpers). We need to remove that duplication.
- However, for the perf reasons explained above, we cannot simply remove the 'string' field type in favor of 'text', and state that current 'string' fields will be 'text' fields in "raw" mode.
- Additionally, "a raw string" and "a string + a format" really are two different things in a data model - and you need to pick one upfront anyway, since there is no changing your mind and switching from one to the other once there is data in there : you cannot arbitrarily assign a format to a string that was just a raw string so far, and you cannot decide to suddenly treat a string with a format as a raw string, ditching its previous format.

Those really are two (related but) separate field types. I'm really not in favor of artificially smushing them in the first UI screen, to then offer a secondary choice in a second screen. The first screen list field types - if there are two separate field types, they should be listed as two options; I don't see ourselves bending the UI to make an exception here, sounds like a nightmare implemetation-wise.

If we really are worried about adding new options here, and need a way to make "similar / related" field types more obvious, then as mentioned above, adding optgroups to that list is easily doable (in a separate issue :-p). But let's not artificially "merge" field types just to avoid adding options in that list.

I agree with the sentiment in #74. This issue is about code simplification and prevention of an entire class of security problems inherent to the current approach.

I also see Bojhan's concerns. But I think those concerns essentially boil down to "we should improve Field API's UI". The exact same reasoning (step 1: text, step 2: plain or rich) can be applied to other fields: File and Image (step 1: media, step 2: field or image). Similarly for number. And so on.

Improving the Field API UI is out of scope for this issue. This patch is more than big enough already. Last but not least: Field API UI improvements like this one are a perfect example of an improvement we can add in Drupal 8.1.x!

P.S.: https://www.drupal.org/project/hierarchical_select :)

Well, I gave my review. If improving the UI is not in scope, I don't want to spend my time discussing it. This will make the Field UI more difficult to use, its already crappy to use, so just more crap :) Please stop requesting usability reviews, of UI's that cant change anyway.

Fundamentally I disagree with the stance that this choice should be exposed at the top level of the UI. There is technically no reason, we can't do it on the second level (as I am reading here) other than a preference to expose everything upfront in one UI.

Interdiff looks great, all concerns have been addressed AFAICT, so back to RTBC!

@Berdir From my understanding, it is a preference to have the raw/formatted decision up-front and not in the field setting wizzard step. This because the way Field UI is constructed, you make this fundamental decision upfront and it cannot be changed/made when configuring the field type. From my point of view, is this architectural decision plain wrong. Users should be able to choose the generic "type" upfront like Text, Image, Number etc. and specify specific data settings in the next step (field settings), after which the field is created and people go and configure the form element display/rules. This is the form builder UI model, which I think maps much more closely what site builders think - as Acquia verified in 2011 (usability testing of their form builder model). The decision here moves, another step away - from having a sensible UI in place.

My concerns have not been addressed, and I am pretty sure this will just go in as is :)

#81: It doesn't move us further away, it merely prevents a security problem and re-inforces the current model. I think it's rather trivial to still create the UI you're describing, possibly in Drupal 8.1.0: just add a "human_field_type" annotation to Field plugins, that will allow you to categorize all of the "textual" fields under "Text", which will allow you to choose a "human field type" (text, image, video and whatever other categories) as a first step and then in the second step only see those fields that are annotated with that human field type. Easy :)

I agree with @Bojhan there are UI concerns - but otoh we should all be concerned about the Field UI. Hopefully in 8.1 we can do something like what @Wim Leers suggests in #82 - anyone care to open an issue?

Committed 57926bf and pushed to 8.0.x. Thanks!

Filed #2340305: add a "human_field_type" annotation to Field plugins

Related : #2344979: Rename field type list_text to list_string