Add Textarea option to normalize newlines to \n

Could we just turn this on by default? Is there a reason we might want to keep \r\n instead of \n?

I can't think of a reason when the line endings actually matter. They don't otherwise matter in PHP and nl2br() accepts any line ending style. The HTML5 spec suggests that web browsers use only \n internally to represent line breaks and then normalize to send \r\n as the value. There is however the chance of some custom code that interprets the line breaks in some way and expects them to be \r\n.

I think the BC way of doing this would be to add this flag, warn users that this will change in Drupal 10 if the flag is not set, and then remove the flag (or change the default to TRUE) in Drupal 10.

But then, is this the right thing to do for all textareas, or is this really only important for ones that deal with config entities? Instead could typed data be responsible for normalisation for storage?

This is the MR as of commit b02df751 in patch format, not going to run tests as it'd be redundant.

This changes the patch so that the default on the new option is TRUE instead of FALSE.

The interdiff in #13 was made incorrectly, this is the correct one.

FYI #13 works with core textarea fields, as used on the Account Settings page, but does not work with Webform's settings pages.

I added an issue to discuss supporting this in Webform: #3239878: Support multiline config export via #3202631

FYI I opened an issue for the core bug that can cause the blank last line of config files to be removed: https://www.drupal.org/project/drupal/issues/3240104

Assume the MR is the one to review here (we need the default FALSE for backward compatibility), but the tests are failing.

How's about this?

A minor change to make sure the string has a lineline character at the end.

$value = preg_replace('~\r\n?~u', "\n", $value);
Why this and not the same code as in Drupal\filter\Element\ProcessedText::preRenderText()? Better to be consistent I guess.

The #normalize_newlines option is an interesting one and a very usable one in my mind. Drupal has so many textfields that are used to input a list of one-liners. In those cases it's very helpful to normalize the set of items. The form with the textfield has the knowledge wether this normalisation is a good idea. The configuration layer cannot know if the CRLF newline has any special meaning.

I wonder if core has some places where this type of normalization is already being executed on textarea. It would be good then to implement this in those case.

This is needed for DX (Developer eXperience), particularly, I have a very large webform, while it does import and export with PostgreSQL no problem, it crashes on MySQL locales_table column too small unless the \r\n is normalized to \n which core handles now see 2844452

as a fix for the locales table crashing in MySQL, a continuation of #2844452: Export configuration YAML strings as multiline

this is because my setup is using more than one language and the config yaml translations are being stored in the locales_source table which on mysql will crash on 64kb+ long strings unless the line endings are normalized.

The issue is, the GUI tools inside the webform module are generating \r\n , some are, some aren't. this is for things like css entry and js entry for webforms and webform confirmation message configuration.

So it keeps reverting to \r\n even though I'm wanting \n.

so, for now, if I want to import this onto a Drupal install using a MySQL type db I need to run my super huge webform yml through dos2unix THEN process it (every !/$**/ time).

This is tedious and can be very frustrating for those that haven't figured out the bug because they could be stumped completely by it.

With that said, postgresql handles this like a breaze because I believe it just stores a blob for these configs when storing to the locales_source table.

Based on patch #18, modified as per comments in #24, using same strategy already implemented in core/modules/filter/src/Element/ProcessedText.php
which is consistent with what is already being used by core.

Patch works well.

I applied the core patch, then added

'#normalize_newlines' => TRUE,

to the css_editor/css_editor.module contrib module form markup

then I rebuilt cache, edited the theme css (as per css_editor added functionality)

saved the css

then exported css

before fix the exported yml looked like '****'

css: "@charset \"utf-8\";\r\n/* CSS Document */\r\n\r\nbody.path-search-facet .view-display-id-page_1 .view-content > .views-row {\r\n  display: block;\r\n}

Now the output yml generated by drush cex is legible and looks like:

css: |
  @charset "utf-8";
  /* CSS Document */

  body.path-search-facet .view-display-id-page_1 .view-content > .views-row {
    display: block;
  }

I suspect the patch will pass testing.

+++ b/core/lib/Drupal/Core/Render/Element/Textarea.php
@@ -58,7 +60,13 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+        // Convert all Windows and Mac newlines to a single newline, so filters only

This comment line here exceeds the 80char limit.

Hi I addressed the comment #33 and clear all the warning which shown earlier patch, so i uploaded the patch along with the interadiff.

@sahil did a code review and there are a few observations.
- Unrelated changes mentioned in #34 have not yet been addressed in the patch above
- The comment can probably be better worded

+        // Convert all Windows and Mac newlines to a single newline,
+        // so filters only need to deal with one possibility.

- We should break the comment closer to 80 chars

The patch in #34 doesn't apply to 10.3, so re-rolled it against both 10.3.x (for those of us who need this patch to be applied to their sites) and 11.x branches.
Also, removed the unrelated change and split patches to test and test+fix.

Added two MRs (test only and test + code change), let's see if tests still passes.

Thanks, looks good to me

This seems good but still needs a change record.

Small code update + added a change record. Please review

It looks like the test is failing.

One test still fails

---- Drupal\Tests\Core\Render\Element\TextareaTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
[31mFail      Other      phpunit-367.xml      0 Drupal\Tests\Core\Render\Element\Te
[0m    PHPUnit Test failed to complete; Error: PHPUnit 10.5.30 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.11
    Configuration: /builds/issue/drupal-3202631/core/phpunit.xml.dist
    
    ..WWW..                                                             7 / 7
    (100%)
    
    Time: 00:00.038, Memory: 8.00 MB
    
    3 tests triggered 1 PHP warning:
    
    1)
    /builds/issue/drupal-3202631/core/lib/Drupal/Core/Render/Element/Textarea.php:66
    Undefined array key "#normalize_newlines"
    
    Triggered by:
    
    * Drupal\Tests\Core\Render\Element\TextareaTest::testValueCallback#2
     
    /builds/issue/drupal-3202631/core/tests/Drupal/Tests/Core/Render/Element/TextareaTest.php:22
    
    * Drupal\Tests\Core\Render\Element\TextareaTest::testValueCallback#3
     
    /builds/issue/drupal-3202631/core/tests/Drupal/Tests/Core/Render/Element/TextareaTest.php:22
    
    * Drupal\Tests\Core\Render\Element\TextareaTest::testValueCallback#4
     
    /builds/issue/drupal-3202631/core/tests/Drupal/Tests/Core/Render/Element/TextareaTest.php:22
    
    OK, but there were issues!
    Tests: 7, Assertions: 7, Warnings: 1.

The test was working earlier, back when it was this:

  public function testNormalizeNewlines() {
    $form_state = $this->prophesize(FormStateInterface::class)->reveal();
    $element = ['#normalize_newlines' => TRUE];
    $input = "some\r\ndifferent\rline\nendings";
    $expected = "some\ndifferent\nline\nendings";
    $this->assertSame($expected, Textarea::valueCallback($element, $input, $form_state));
  }

It's green now.

I've refactored test a bit to be more robust and support more cases.

1) Drupal\Tests\Core\Render\Element\TextareaTest::testValueCallback with data set #5 ('some\ndifferent\nline\nendings', 'some\r\ndifferent\rline\nendings')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 #Warning: Strings contain different line endings!
-'some
-different
-line
+'some
line
 endings'

Ran test-only feature to see coverage
Summary appears complete
Code change itself seems pretty straight forward

LGTM!

There are no unanswered questions here and is does look straight forward as said in the previous comment.

Unfortunately, one of the comments was not wrapped correctly. I updated that one and made a change to another.

I then reviewed the test and found that most of the defaults for the element did not need to be set. That lead me to rework the test and the data provider. The data provider now sets the element configuration as needed and the structure is changed as well.

This now needs another review.

Coming back to check the change record. I see no evidence here that anyone has reviewed the change record. I took a brief look and was overwhelmed by the second paragraph with a long sentence that is hard to parse. Remember, the readers of change records will have varying levels of skills with the English language.

Setting to needs work for a review of the change record.

The comment changes look good to me.

Big rebase after #3525331: Reuse element plugins as object wrappers around render arrays.
I rewrote the CR.

Changing from a task to a bug, as evidenced by #3536318: \r\n on the config form causes the property_restrictions to not be followed

I'm pretty sure there is duplication between this issue and #3226410: CRLF causes mismatch between JS and form maxlength validation.

1 small comment on the MR.

Also, @alexpott brought up in #3226410: CRLF causes mismatch between JS and form maxlength validation #27 that normalizing "\r\n" to "\n" will cause values to be stored differently between form and JSON:API submissions when textareas are used in entity form widgets. Should core widgets set their textarea's #normalize_newlines property to FALSE?

Small comments on the MR with 1 question.

Pushed a commit addressing #64 with a test and also the MR comment about type declarations.

I think #3226410: CRLF causes mismatch between JS and form maxlength validation can be closed as a duplicate now and credit ported here.

Ready for review again.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Justification:

1. The solution is in scope
2. Has tests
3. Tests are passing
4. Tests only fails

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased and applied outstanding suggestion.

Rebase seems good.

@tim.plunkett, thanks for rewriting the change record. I have reviewed it and it is very clear what this change is.

I read the MR and changed 2 comments which didn't read correctly. Leaving at RTBC

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The MR rebased clean with no conflicts, so back to RTBC.

I saw this comment in the merge request:

<?php
      // Since field values submitted via JSON:API will not have "\r\n"
      // normalized to "\n", do not normalize the newlines in the form element,
      // so that field values saved to storage are the same whether submitted
      // by form or JSON:API.
      '#normalize_newlines' => FALSE,
?>

But all browsers on all OSes themselves normalize textarea text newlines to `\r\n` when they bundle it up to send as form data. So if you send it to JSON:API with `\n` and then edit that entity and save the form via the admin UI, that value will receive `\r\n` instead on Mac, Windows, Linux, doesn't matter. `\r\n` is the HTML standard.

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#...

They recently changed the spec (2021) to change how interacting with textareas works via JS.

https://github.com/whatwg/html/pull/6697
https://github.com/whatwg/html/issues/6647

But that does not matter when the form is eventually submitted via multipart/form-data, that still normalizes everything to CRLF.

Re #77, the discussion about standard browser normalization behavior to CRLF and JSON:API started in #3226410-27: CRLF causes mismatch between JS and form maxlength validation. The concern is that JSON:API submissions with data containing "\r\n" will not be normalized to "\n", resulting in data being stored differently depending on submission method.

I think some of that discussion might be under the assumption that application/json submissions are normalized to CRLF the same as multipart/form-data, application/x-www-form-urlencoded, or text/plain. It seems like that might not be the case? In which case, the line endings for text content submitted via JSON:API might vary based on the OS where the content was authored?

I am going off of old school Core REST with the standard json format, not JSON:API, but when someone sends a string with only LF in it as application/json, that is what gets stored in the database. If I then edit that entity in the UI, and save it, those LFs get normalized to CRLF due to the HTML spec browsers enforce.

Currently, when the values are submitted via a form I am comparing the new value to the old value and if they match exactly after line ending normalization, I keep the old value to respect whatever line endings already exist in the database. Otherwise I normalize to LF, which breaks fewer things than CRLF afaik.

I would normalize on the consumer side, but it is a generic field and should respect whatever line endings the user sent the text in because it gets used as a source to replace certain tokens in files. Could be LF, CR, CRLF, whatever was sent should be kept. That is an edge case, of course.

But, yes, seems like the OS matters. And what sends it matters. I think application/json has no spec or enforcement, and even if it did not all clients capable of sending application/json would follow it. Postman, curl, a JS framework that does or does not normalize line endings, wrap=hard being turned on in a textarea, the list goes on.

It should be easy enough to test: POST some data with just LF to a textarea via JSON:API or Core REST JSON, print it somewhere as a json encoded string to see the lack of carriage returns and presence of line feeds, then save it via the admin interface and you should see that carriage returns have snuck in. And this should happen on any OS, in any browser.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed the test since summary is no longer shipped.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased and set issue version to main.

Attached a static copy of the current patch for 11.x

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Back to RTBC.

There's a merge conflict that needs to be addressed. Also I feel as though this issue should introduce the capability and not change the default behaviour then we should discuss the default in a followup. For me this has a non-zero chance of breaking something somewhere. So we should be a bit cautious. There hasn't been a discussion on the issue to say why we've not followed @longwave's suggestion in #7.

I fixed the merge conflicts but the concerns from #89 still need to be addressed.