Regression: line-breaks are mangled by \Drupal\Core\Utility\Mail::htmlToText()

Patches for d7 and d8:

You can help by posting a review of the patch in #1. An acceptable review would:

• Confirm that the patch actually solves the problem. Describe a reproducible test where the current code fails and the patched code succeeds.

• Change the issue status from "Needs Review" to "Reviewed and Tested By the Community".

If, after thorough testing, you cannot in good conscience affirm the above, please explain what is lacking in the current patch and I will do my best to address your concerns.

@rrcjab -- Yes, it's the same patch; just re-rolled against a fresh 8.x checkout.

@rrcjab -- The next step is someone other than the patch-writer having sufficient confidence and fortitude to mark the issue RTBC. After that, well... (looking) The oldest patch in a similar status has been waiting for less than a month.

Patch in #1 still applies, and the the other issue may still be in debate when D9 ships. Let's fix what we broke in D7 first.

It is still a duplicate of #299138:

No, it's not. This is a only a single-line quick-fix reversal of a known regression introduced in D7. #299138 is a much larger issue, which no one is willing to commit until it reaches a level of perfection that may not happen in my lifetime.

@#15 -- Added tests, made them pass, (by "expecting" whatever results were generated), then removed the ones which are totally n/a.

support for tables... really?

Wasn't that hard, actually, since I already had a fully-parsed DOM tree.

Looks like testbots are fixed.

Dunno; tests pass locally and the testbot is not displaying any of the $this->verbose() output. Trying again...

Moved the verbose() text back into the assertion message so I can figure out why the tests are failing.

Grrr....

#31: drupal_html_to_text-fix_regression-1130198-31.patch queued for re-testing.

@#34 -- I dunno either, but they did. The tests in #299138-134 have url() calls, and they passed, too. Other core tests also contain url() calls:

• SiteMaintenanceTestCase::testSiteMaintenance()
• DateTimeFunctionalTest::testDateTypeConfiguration()
• DateTimeFunctionalTest::testDateFormatConfiguration()
• TokenReplaceTestCase::testSystemSiteTokenReplacement()
• PageEditTestCase::testPageEdit()
• NodeTypeTestCase::testNodeTypeEditing()
• NodeTokenReplaceTestCase::testNodeTokenReplacement()

There are more in openid.test, book.test, profile.test, update.test, user.test, path.test, image.test, comment.test, color.test, rdf.test, file.test, locale.test, aggregator.test, translation.test, filter.test, trigger.test, search.test, simpletest.test, xmlrpc.test, bootstrap.test, common.test, file.test, path.test, ajax.test, form.test, menu.test, and taxonomy.test.

Also, could you please add @todo everywhere in the test where you think the current behavior is not the intended one?

That would be just about everywhere, all right. Working on it...

Switched to DrupalWebTestCase and added @todo lines. Removing the "Quick fix" tag, as it no longer applies.

#39: drupal_html_to_text-fix_regression-1130198-39.patch queued for re-testing.

Bot is green.

Will do, after I re-test locally. @sun gave me some pointers on IRC. My local test must run with clean urls turned off, and with Drupal installed in a subdirectory, else the results aren't valid.

Fixed.

#46: drupal_html_to_text-fix_regression-1130198-46.patch queued for re-testing.

Issue and patch summary

October 15, 2009

Dries committed a patch from #407452-58.

May 21, 2010

Arjenk posted #299138-48 which claimed that part of the patch had broken drupal_html_to_text() functionality:

diff -u -p -r1.25 mail.inc
--- includes/mail.inc	1 Sep 2009 17:40:27 -0000	1.25
+++ includes/mail.inc	15 Oct 2009 05:58:51 -0000
@@ -466,7 +496,7 @@ function drupal_html_to_text($string, $a
         $chunk = $casing($chunk);
       }
       // Format it and apply the current indentation.
-      $output .= drupal_wrap_mail($chunk, implode('', $indent)) . "\n";
+      $output .= drupal_wrap_mail($chunk, implode('', $indent));
       // Remove non-quotation markers from indentation.
       $indent = array_map('_drupal_html_to_text_clean', $indent);
     }

August 1, 2010

Kscheirer posted #299138-50, explaining:

What wasn't obvious (because we had no tests for this function!) was that many of the tags were relying on that extra \n to format themselves properly. Stuff like h1, h2, dl, br, p, ol, ul, li, dl, dt, dd, etc. I think your patch was trying to make this function more general - but it doesn't seem to fulfill its original purpose now. Also _drupal_html_to_text_pad() is doing a substr, relying on the fact that there was an extra \n to throw away.

August 31, 2010

After some discussion, Kscheirer summarized in #299138-56:

Agreed, we're back to square one.

• Define what exactly this function is supposed to do
• Write tests for it
• Fix the function to pass tests

April 17, 2011

Pillarsdotnet posted a patch in #299138-75 to reverse the chunk that caused the initial problem.

Sun changed the status back to "needs work" with #299138-76, saying:

It would tremendously help if people would re-roll the proper patch. See #57.

Pillarsdotnet posted a revised patch in #299138-84 which passed all but one of the proposed tests, saying:

Removed the "Full HTML Document" test because:

1. I couldn't get it to pass no matter what I did.
2. I think it's going about things the wrong way.

Sun rejected the patch in #299138-86, saying:

Removing a test that has been added for a good reason is a bad idea. The stated reasons are, in fact, critical reasons to keep it.

Pillarsdotnet opened #1130198 to offer the patch from #299138-75 as an interim solution.

April 23, 2011

Damien Tournoud closed #1130198-9, saying:

This is an obvious duplicate of #299138: Improve \Drupal\Core\Utility\Mail::htmlToText()

April 17 - June 4, 2011

An additional 58 patches were posted to #299138 (34 of which passed all tests), and the patch size grew from 9.5Kb to 42.5 Kb as newly-requested features and tests were added.

June 4, 2011

Pillarsdotnet re-opened #1130198-11, saying:

Let's fix what we broke in D7 first. ... This is a only a single-line quick-fix reversal of a known regression introduced in D7.

After some discussion, Damien Tournoud agreed, and requested in #1130198-15:

Can we incorporate some of the tests from the other issue?

June 5, 2011

Damien Tournoud added in #1130198-36:

Also, could you please add @todo everywhere in the test where you think the current behavior is not the intended one?

June 6, 2011

Pillarsdotnet posted a patch in #1130198-46 that appears to resolve all objections raised so far.

Adding tests-only and tests+fix patch to highlight exactly what this patch changes.

(sigh)

#50: drupal_html_to_text-fix_regression-1130198-tests+fix.patch queued for re-testing.