Use the string field type for the node title field

TextDefaultFormatter just prints $item->processed, which doesn't exist for string items ?

+++ b/core/modules/text/text.module
@@ -190,3 +190,11 @@ function text_filter_format_update($format) {
+function text_field_info_alter(&$info) {
+  $info['string']['default_widget'] = 'text_textfield';
+  $info['string']['default_widget'] = 'text_default';
+}

I guess the second is supposed to be default_formatter. ;)

That said, what is a String field's widget if Text module is not installed?

[edit: following up on #2 / crosspost with #3, #4, #5 :-)]

And, we do want check_markup() to run on node.title / other string fields.

If moving the check_markup() to a ->processed computed property was deemed ok for configurable "non formatted, short text fields", then I'm not sure why we'd see this as too costly for node.title ?

A more radical move could be to state that only "long texts" can be have a format, remove the 'text_processing' setting from the 'text' field type, and then de facto merge it into the 'string' field type ?

string -> stores a VARCHAR(N)
text_long -> stores a BIGTEXT + a format (can be configured to not use format and be treated as plain text)
text_long_with_summary -> same, with a summary

Thoughts ?

Coming from #1856562: Convert "Subject" and "Message" into Message base fields

+++ b/core/modules/text/text.module
@@ -190,3 +190,11 @@ function text_filter_format_update($format) {
+function text_field_info_alter(&$info) {
+  $info['string']['default_widget'] = 'text_textfield';
+  $info['string']['default_formatter'] = 'text_plain';
+}

why not just move textPlain to Core and the same for widget? I'm trying to make it in #2150511: [meta] Deduplicate the set of available field types but first moved them to field module

@berdir: doh, right, I meant check_plain(), not check_markup(). Face slap.

If we're doing this though, maybe we should go the other way around and move TextPlainFormatter into Core as StringFormatter, make it the official formatter for the 'string' field type, and then have text.module add it as a possible formatter for text fields too.
That would remove a dependency spot on node.module hardcoding a formatter provided by text.module ?

(er, well, basically, what @andypost says in #8)

I think we have use cases for both long and short strings with and without format. There's an issue to add a StringLong somewhere.

Still a bit skeptical about the real use case for "text <= 255 chars with a format", but ok.

Then I guess we could still more cleanly separate:
- 'string' / 'string_long' = "plain text" (just a VARCHAR or TEXT)
- 'text' / 'text_long' / 'test_with_summary' = a "formatted text" (VARCHAR or TEXT + a format + possibly a summary TEXT)

Removes the 'formatted_text' bool setting on text fields, and you lose the ability to switch an existing field back & forth between 'formatted' & 'non formatted', you need to chose upfront by chosing a field type. Not sure that ability really made any sense.

@sun, interested in your thoughts on this :-)

Implementation of default string widget & formatter in Core/* is possible but how they could be used without field and entity modules?

Thanks, patch makes a lot of sense that way IMO - & the cleanups in _info_alter() are really welcome.

Then I guess TextfieldWidget should follow along ? (/me runs)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeValidationTest.php
@@ -54,7 +54,7 @@ public function testValidation() {
-    $this->assertEqual($violations[0]->getMessage(), '<em class="placeholder">Title</em>: the text may not be longer than 255 characters.');
+    $this->assertEqual($violations[0]->getMessage(), 'This value is too long. It should have <em class="placeholder">255</em> characters or less.');

This is the "duplicate violation" thing we've already discussed on node.title, right ? It doesn't mean the actual error message displayed becomes different ?

@yched yep, this fires entity validation constraint, see commited #2002168: Convert form validation of terms to entity validation

Moving the textfield widget would be really good for consistency - but yeah, can be a followup I guess.

The field label missing in validation messages is a UX regression though :-/
I think we have a more general issue about those, but without a proper plan yet IIRC.

Meanwhile, couldn't we simply have StringItem::getConstraints() do the same as TextItem::getConstraints() and provide a custom 'maxMessage' ?

Suppose we need StringWidget and StringLongWidget could be done in one of follow-ups.
So anyway summary needs rewrite

Added StringWidget and inherited TextfieldWidget from it.
Once text module enabled string default widget should not be changed, so removed alter

Thanks @andypost.

- TextfieldWidget::formElement() could build on parent::formElement() and simply add the 'format' input then.

- Node::baseFieldDefinitions() needs to be updated to state that 'title' uses 'string' widget instead of text_textfield
(patch works because of the "fallback to default widget" mechanism in WidgetPluginManager::getInstance())

Other than that, looks good (aside from the bit about StringItem::getConstraints() from #17 ?)

so all #22 addressed

Fix more tests

Fix last tests

Thanks @andypost !

Looks good if green.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItem.php
   @@ -71,7 +73,12 @@ public function getConstraints() {
   +            'maxMessage' => t('%name: the string may not be longer than @max characters.', array('%name' => $this->getFieldDefinition()->getLabel(), '@max' => $max_length)),
   

   Will this constraint error message be displayed to end-users?

   If so, "the string" is a very technical/geeky term - we should retain "the text".

   Alternatively, do we actually need to use this linguistic prefix?

   '%name may not be longer than @max characters.'

   ...cuts it, no? Shorter, better, faster. :)

2. +++ b/core/modules/email/email.module
   @@ -35,12 +35,7 @@ function email_field_info_alter(&$info) {
   -  if (\Drupal::moduleHandler()->moduleExists('text')) {
   -    $info['email']['default_formatter'] = 'text_plain';
   -  }
   -  else {
   -    $info['email']['default_formatter'] = 'email_mailto';
   -  }
   +  $info['email']['default_formatter'] = 'email_mailto';
   

   Why are we changing the default formatter from plain string to a mailto link?

3. +++ b/core/modules/text/text.module
   @@ -190,3 +190,12 @@ function text_filter_format_update($format) {
   +function text_field_formatter_info_alter(&$info) {
   +  $info['string']['field_types'][] = 'text';
   +  $info['string']['field_types'][] = 'text_with_summary';
   +  $info['string']['field_types'][] = 'text_long';
   +}
   

   I thought the differentiation between string and text would be that the latter has a format?

   If you render a text value but ignore the format, then it could be possible to generate insecure output (lots of possible edge-cases).

1) +1
2) when we enable email module, suppose better to make proper formatter default, all stored formatters in entity displays are not changed
3) string formatter could be re-used for all text types, it makes strict check_plain for value, so format is not used.

@sun:

1. "%field_label : [error message]" is the standard shape of error message for field errors, makes the phrasing list of errors more consistent in case there are several. Do we really want to do a one-off change here ?

3.

"the differentiation between string and text would be that the latter has a format"

... the latter *can* have a format. 'formatted_text' is an instance-level field setting. You can have long texts with a NULL format, and using a "check_plain" formatter makes sense on those.

That's the current state of text field types in D7 / D8. In #9 I suggested we might want to more cleanly separate formatted and non-formatted text types:

Then I guess we could still more cleanly separate:
- 'string' / 'string_long' = "plain text" (just a VARCHAR or TEXT)
- 'text' / 'text_long' / 'test_with_summary' = a "formatted text" (VARCHAR or TEXT + a format + possibly a summary TEXT)

This would remove the 'formatted_text' bool setting on text fields, and you lose the ability to switch an existing field back & forth between 'formatted' & 'non formatted', you need to chose upfront by chosing a field type. Not sure that ability really made any sense.

But yes, as @Berdir pointed in #11, that's a functionnality loss.

1) fixed
2) email defined as core field type so string formatter and widget applicable by default
3) needs follow-up

PS: searching for field error messages I found scary places like '!name' => $element['title']['#title'] not sure about SA, but needs some follow-up t unify errors

• Fix broken test
• telephone module reverted to default formatter
• Removed unnecessary changes in TextItem

Clean-up namespace

I only skimmed the patch, so leaving to someone else to RTBC when it's ready, but just wanted to chime in a +1 for this issue, and nothing jumped out at me as problematic.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
@@ -2,31 +2,30 @@
- *   id = "text_plain",
+ *   id = "string",

This will need a D7->D8 migration issue opened. What's our process for that? Also, tagging as "beta target" so as to avoid needing a beta->beta hook_update_N() implementation.

Minor - form interdiff #35:

+++ b/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItem.php
@@ -55,16 +55,16 @@ public static function schema(FieldDefinitionInterface $field_definition) {
   public function getConstraints() {
+    $constraint_manager = \Drupal::typedDataManager()->getValidationConstraintManager();
     $constraints = parent::getConstraints();
 
     if ($max_length = $this->getSetting('max_length')) {
-      $constraint_manager = \Drupal::typedDataManager()->getValidationConstraintManager();
       $constraints[] = $constraint_manager->create('ComplexData', array(

Not sure why #35 reverts this from the previous iterations, this sounded like a good idea ?

Other than that, which is not a blocker, looks good.
@sun's concerns have been addressed, so back to RTBC ?

@yched that was reverted because @berdir pointed that we should not touch files that are not affected.

@andypost : right, makes sense

Disclaimer: I don't want to derail this issue or delay its resolution, I'm just trying to catch up after a period of forced inactivity.

I was wondering whether this will allow to have HTML in titles. When I mentioned this use a case at the beginning of D8 life cycle, I was told widgets/formatters on base fields would be the answer, but I am not sure this is still the case.

Yes, there are some tricky aspects that need to be taken into account, but I thought that we could have EntityInterface::label() (the human-readable identifier) always return a plain string and, for instance, $node->title->value (the actual content) return the user-entered value.

Anyway, since it seems this issue does not affect the use case above, I'll just shut up and wait for commit :)

2198917-node-title-36.patch no longer applies.

error: patch failed: core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php:53
error: core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php: patch does not apply
error: patch failed: core/modules/node/lib/Drupal/node/Entity/Node.php:377
error: core/modules/node/lib/Drupal/node/Entity/Node.php: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php:101
error: core/modules/system/lib/Drupal/system/Tests/Entity/EntityTypedDataDefinitionTest.php: patch does not apply

re-roll

back to rtbc

Committed 2187fd1 and pushed to 8.x. Thanks!

In one of the new files we were using the deprecated check_plain()

diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
index 01e2e8c..d5e0c17 100644
--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
@@ -7,6 +7,7 @@
 
 namespace Drupal\Core\Field\Plugin\Field\FieldFormatter;
 
+use Drupal\Component\Utility\String;
 use Drupal\Core\Field\FormatterBase;
 use Drupal\Core\Field\FieldItemListInterface;
 
@@ -36,7 +37,7 @@ public function viewElements(FieldItemListInterface $items) {
     foreach ($items as $delta => $item) {
       // The text value has no text format assigned to it, so the user input
       // should equal the output, including newlines.
-      $elements[$delta] = array('#markup' => nl2br(check_plain($item->value)));
+      $elements[$delta] = array('#markup' => nl2br(String::checkPlain($item->value)));
     }
 
     return $elements;