When adding a "Long text" (textarea) field with "Default" formatter and entering some text containing newlines, those newlines should be retained.

When no text format is attached to a field value, then the text value is passed through check_plain(), so users don't even have a chance to enter newlines in any other way.

Files: 
CommentFileSizeAuthor
#24 text-plain-1152216-24.patch1.25 KBericras
PASSED: [[SimpleTest]]: [MySQL] 40,123 pass(es).
[ View ]
#21 text.plain_.16.patch-before.png43.2 KBandymartha
#21 text.plain_.16.patch-after.png44.46 KBandymartha
#17 text.plain_.16.patch12.99 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,114 pass(es).
[ View ]
#9 drupal8.text-field-value-newline.9.patch2.74 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,122 pass(es).
[ View ]
#7 drupal.text-field-value-newline.4-test-only.patch1.69 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,593 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#4 drupal-1152216-04-text-new-line.patch2.7 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 33,580 pass(es).
[ View ]
drupal.text-field-value-newline.0.patch1.01 KBsun
PASSED: [[SimpleTest]]: [MySQL] 29,463 pass(es).
[ View ]

Comments

yched’s picture

Status:Needs review» Reviewed & tested by the community

Makes sense. Right now, 'long text' fields set to 'unformatted' are useless without being able to handle newlines.

catch’s picture

Issue tags:+needs backport to D7

Assuming this tag was missed, if not feel free to remove it.

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Makes sense, but let's get some tests added.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new2.7 KB
PASSED: [[SimpleTest]]: [MySQL] 33,580 pass(es).
[ View ]

This patch updates test.text so that the existing tests account for new lines in the text_plain format.

sun’s picture

Status:Needs review» Needs work

Sorry, but no, that's not desired here.

All that's left for this issue is to write unit tests for the patch in #0.

jhedstrom’s picture

The changes to the existing tests make them test against the patch in #0...if you apply the patch from #4, revert changes to text.module, and run the tests, they fail. They pass with the patch re-applied to text.module. Is it preferable to write an entire new test method to test for line breaks?

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] 33,593 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

ouch, I'm terribly sorry, my fault. Didn't look closely enough. However, no time to review now. But we can testbot-approve the tests.

Status:Needs review» Needs work

The last submitted patch, drupal.text-field-value-newline.4-test-only.patch, failed testing.

sun’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs tests
StatusFileSize
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 34,122 pass(es).
[ View ]

Alright, test-only patch in #7 confirmed that #4 actually fixes the bug.

Re-rolled against latest HEAD (/core directory change) and calling this RTBC.

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/field/modules/text/text.moduleundefined
@@ -316,7 +316,7 @@ function _text_sanitize($instance, $langcode, $item, $column) {
-  return $instance['settings']['text_processing'] ? check_markup($item[$column], $item['format'], $langcode) : check_plain($item[$column]);

I reckon both places where we're using nl2br could use a comment to explain why?

altavis’s picture

Hi

I think it is wrong problem discussed and fixed here. It's not about converting new lines to <br /> on every plain text format. There's an option in text formats configuration called "Convert line breaks into HTML (i.e. <br> and <p>) " and the problem is that it's not honoured. Sorry if I'm wrong but this is my opinion after seeing the patch.

Please see this:
http://drupal.org/node/1095838

sun’s picture

@altavis: As explained in the OP, this issue is about the case where no text format and thus no filters are applied in the first place. Consequently, your concerns in #11 don't make much sense.

altavis’s picture

Yep, you are right Sun. Thanks for patch.

JvE’s picture

Status:Needs work» Closed (duplicate)
sun’s picture

Status:Closed (duplicate)» Needs work

If anything, then the other issue is a duplicate of this here. Multiple issues seem to have been merged into it though, so it might be about additional other aspects.

David_Rothstein’s picture

I closed that one as a duplicate.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new12.99 KB
PASSED: [[SimpleTest]]: [MySQL] 50,114 pass(es).
[ View ]

Completely redone against HEAD. Including new Formatter\TextPlainUnitTest.

Created a follow-up issue for #1902098: Introduce DUTB helper methods for working with rendered content + FieldPluginUnitTestBase

sun’s picture

Component:field system» text.module
sun’s picture

#17: text.plain_.16.patch queued for re-testing.

sun’s picture

I think this patch properly corrects the situation and comes with sufficient test coverage.

I'm aware that the test helper methods and stuff are slightly unrelated here, but I really could not stand all of the custom + convoluted test setup procedures anymore, so I created these here as a starting point for #1902098: Introduce DUTB helper methods for working with rendered content + FieldPluginUnitTestBase, and I want to move forward over there as soon as this patch has landed.

andymartha’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new44.46 KB
new43.2 KB

In a fresh installation of Drupal 8.x-dev on March 6th, 2013, the problem in the original issue was verified. In a content type, adding a field of:
- a "FIELD TYPE" long text "WIDGET" text area (multiple rows) of "Text processing" plain text, and multi-line content -- when viewed, the content is on one-line.
- a "FIELD TYPE" long text "WIDGET" text area (multiple rows) of "Text processing" Filtered text (user selects text format), and multi-line content -- when viewed, the content is on multi-line. See screenshots.

After applying patch text.plain_.16.patch in #17 by sun,
- a "FIELD TYPE" long text "WIDGET" text area (multiple rows) of "Text processing" plain text, and multi-line content -- when viewed, the content is on multi-line.
- a "FIELD TYPE" long text "WIDGET" text area (multiple rows) of "Text processing" Filtered text (user selects text format), and multi-line content -- when viewed, the content is on multi-line. See screenshots.

Dries’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed to 8.x. Thanks!

yched’s picture

Note that #1932382: Use DrupalUnitTestBase for Field API tests., which was committed shortly after, introduced FieldUnitTestBase, which contains its own (slightly different) versions of assertRaw(), assertNoRaw(), assertText(), assertNoText().

ericras’s picture

StatusFileSize
new1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 40,123 pass(es).
[ View ]

D7 patch (without tests)

webadpro’s picture

Has this been ported to D7?

tstoeckler’s picture

Status:Patch (to be ported)» Needs review

It seemingly has been ported, yes. :-) It is not part of the Drupal 7 development branch, though, yet.
Let's let the bot have a go.

drupov’s picture

The patch from #24 applies perferctly against drupal-7.22

jyee’s picture

Status:Needs review» Reviewed & tested by the community

Also applies cleanly to and works on 7.23

bircher’s picture

The patch from #24 works and produces the result I expected.
so I second @jyee and say RTBC

tsi’s picture

That's got me WTFing for a moment there. I mean - you have 'convert newlines' checked, and it is simply ignored.
Yep, RTBC too.

saiya’s picture

path from #24 working for me too. using d7.23. now my address text field is all with break, not inline anymore.. tqvm

*oh one thing.. if there is core update available. if i apply new update exampel d7.24.. i suppose to apply that patch again?

Majdi’s picture

Patch #24 works for me

bircher’s picture

what needs to happen for this to go into D7, with clear instructions I would be happy to help.

David_Rothstein’s picture

Issue summary:View changes
Status:Reviewed & tested by the community» Needs work

Why weren't the tests backported to Drupal 7?

Also, I don't see how we can commit this to a stable release as is. People could certainly have line breaks in the database that they expect not to display (for example, using the Migrate module to pull in content from who-knows-what source with who-knows-what kind of broken formatting). Maybe a checkbox to control the behavior?

Also not sure about the idea of changing the meaning of the "plain text" formatter (removing the strip_tags behavior) in a stable release...

sun’s picture

Version:7.x-dev» 8.x-dev
Status:Needs work» Fixed
Issue tags:-needs backport to D7

Yeah, you're right. This issue was originally created around the time when 7.0 or so was released, so changing the behavior of the default (plain) formatter shortly after in one of the first point releases would probably have been slightly more acceptable, but today this change would probably break way too many sites.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

figureone’s picture

Since this will never make it into D7 core, here's a simple (but inefficient) way to convert newlines to br tags on text_plain formatted fields using template_preprocess_field in your theme (remember to replace "template" in the function name with the name of your theme):

function template_preprocess_field(&$variables, $hook) {
  if (
    isset($variables['element']['#items'][0]) && (
      !isset($variables['element']['#items'][0]['format']) ||
      $variables['element']['#items'][0]['format'] === 'text_plain'
    )
  ) {
    foreach ($variables['items'] as $index => $value) {
      $markup = isset($variables['items'][$index]['#markup']) ? $variables['items'][$index]['#markup'] : '';
      $variables['items'][$index]['#markup'] = nl2br($markup);
    }
  }
}
esolitos’s picture

I would suggest adding a condition to the code suggested by figureone: $variables['element']['#field_type'] !== 'ds'.
In this way you don't mess with Display Suite fields.