Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT’s picture

Removed all usages of drupal_wrap_mail. Patch attached.

JeroenT’s picture

Status: Active » Needs review

Go testbot!

JeroenT’s picture

Created follow-up issue to remove the function itself: #2359071: Remove drupal_wrap_mail.

rpayanm’s picture

FileSize
2.29 KB
4.48 KB

minor changes

javivf’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Mail/MailInterface.php
    @@ -53,8 +53,10 @@ public function format(array $message);
    +   *     Email bodies must be wrapped.
    +   *     You can use
    +   *     \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    +   *     for smart plain text wrapping.
    

    Not formatted correctly.

  2. +++ b/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
    @@ -358,28 +359,29 @@ public function testVeryLongLineWrap() {
    -   * Tests that drupal_wrap_mail() removes trailing whitespace before newlines.
    +   * Tests that \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    +   * removes trailing whitespace before newlines.
    ...
    -   * Tests that drupal_wrap_mail() does not remove the trailing whitespace from
    -   * Usenet style signatures.
    +   * Tests that \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    +   * does not remove the trailing whitespace from Usenet style signatures.
    
    +++ b/core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
    @@ -17,7 +17,9 @@
    -   * Makes sure that drupal_wrap_mail() wraps the correct types of lines.
    +   * Makes sure that
    +   * \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    +   * wraps the correct types of lines.
    

    Should be a single line. For example the first one could be:

    Tests that trailing whitespace is removed before newlines.
    
    @see \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    
JeroenT’s picture

Issue tags: +Needs reroll
er.pushpinderrana’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.51 KB

Re-rolled.

rpayanm’s picture

Status: Needs review » Needs work
+ * Email bodies must be wrapped. You can use
+ * \Drupal\Core\Mail\MailFormatHelper::wrapMail()for smart plain text
+ * wrapping.

\Drupal\Core\Mail\MailFormatHelper::wrapMail()-space here-for

gaurav.pahuja’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

Added required space.

+ * Email bodies must be wrapped. You can use
+ * \Drupal\Core\Mail\MailFormatHelper::wrapMail() for smart plain text
+ * wrapping.
rpayanm’s picture

Status: Needs review » Reviewed & tested by the community

Look fine for me :)

alexpott’s picture

+++ b/core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
@@ -17,7 +17,8 @@
   /**
-   * Makes sure that drupal_wrap_mail() wraps the correct types of lines.
+   * Makes sure that \Drupal\Core\Mail\MailFormatHelper::wrapMail() wraps the
+   * correct types of lines.
    */

This should be changed to just:

/**
 * @covers ::wrapMail
 */

This is a special format for PHPUnit test method doc blocks that informs code coverage reports what is being tested. The docblock is essentially useless here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs works for #12 and since the replacement docblock in #10 breaks code standards by being multi-line.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
1.48 KB

Made changes as suggested by alexpott in #2358995-12: Remove usage of drupal_wrap_mail(). Patch attached.

rpayanm’s picture

FileSize
4.42 KB
4.42 KB

Fix comments length :)

rpayanm queued 15: 2358995-15.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2358995-15.patch, failed testing.

rpayanm’s picture

Issue tags: +Needs reroll
Alienpruts’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +DrupalCamp Ghent 2014
FileSize
4.02 KB

My first attempt at a reroll.

Had to resolve one conflict in file HtmlToTextTest.php :

use Drupal\Component\Utility\String;
use Drupal\Component\Utility\Unicode;
use Drupal\Core\Mail\MailFormatHelper;
use Drupal\Core\Site\Settings;
use Drupal\simpletest\WebTestBase;

I hope I dit it correctly, if not : please advise on what I did wrong and what I should've done :)

PS : yes, i tested the patch against the 8.0.x branch with git apply --check :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a prioritized change (deprecated function removal) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 547a1a0 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Mail/HtmlToTextTest.php b/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
index de76c13..a6eccee 100644
--- a/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
+++ b/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
@@ -363,6 +363,7 @@ public function testVeryLongLineWrap() {
 
   /**
    * Tests that trailing whitespace is removed before newlines.
+   *
    * @see \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    */
   public function testRemoveTrailingWhitespace() {
@@ -372,11 +373,12 @@ public function testRemoveTrailingWhitespace() {
   }
 
   /**
-   * Tests that \Drupal\Core\Mail\MailFormatHelper::wrapMail() does not remove
-   * the trailing whitespace from Usenet style signatures.
+   * Tests that trailing whitespace from Usenet style signatures is not removed.
    *
    * RFC 3676 says, "This is a special case; an (optionally quoted or quoted and
    * stuffed) line consisting of DASH DASH SP is neither fixed nor flowed."
+   *
+   * @see \Drupal\Core\Mail\MailFormatHelper::wrapMail()
    */
   public function testUsenetSignature() {
     $text = "Hi there!\n-- \nHerp Derp";

Minor fixes on commit.

  • alexpott committed 547a1a0 on 8.0.x
    Issue #2358995 by rpayanm, JeroenT, Alienpruts, gaurav.pahuja, er....

Status: Fixed » Closed (fixed)

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