Problem/Motivation

For header, CC is almost as much as important as To. We need a method for it.

Proposed resolution

Extend interface similar to getTo and MessageTrait implementation.
Let it return multiple.

Later trigger address parsing and return multiple objects.

API changes

New method getCC().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker created an issue. See original summary.

ModernMantra’s picture

Assigned: Unassigned » ModernMantra
Status: Active » Needs review
FileSize
2.31 KB
miro_dietiker’s picture

Status: Needs review » Needs work
  1. +++ b/src/MIME/MessageTrait.php
    @@ -46,4 +46,19 @@ trait MessageTrait {
    +      if (strpos($body, '@') !== FALSE) {
    +        // Extracting body after '@' sign for proper IDN decoding.
    +        $body = explode('@', $body, 2)[0] . '@' . idn_to_utf8(explode('@', $body, 2)[1]);
    +      }
    

    Now this part is repeated 3 times in the MessageTrait. You can create a protected helper as a first step.

  2. +++ b/tests/src/Unit/MIME/MessageTest.php
    @@ -77,6 +77,18 @@ class MessageTest extends UnitTestCase {
    +    $message = new Message(new Header([['name' => 'Cc', 'body' => 'bob_schwammkopf@xn--spuvabob-o4b.com']]), 'I am a body');
    +    $this->assertEquals('bob_schwammkopf@spužvabob.com', current($message->getCC(TRUE)));
    

    The standard is asking us to use example.com as example domain...
    I already created the variant éxample, we should not add too many more. If you are up for fancy terms, use ANYTHING.example.com

ModernMantra’s picture

Status: Needs work » Needs review
FileSize
10.24 KB

By some investigation, IMHO issue create getTo() overlaps with this - reason is that we have repeating code in functions and new helper function (see patch) fixes this issue and another referenced.We can possibly merge getTo() issue so that here we add only API signature. Or maybe i am wrong... :-)

miro_dietiker’s picture

Status: Needs review » Postponed
+      if (strpos($body, '@') !== FALSE) {
+        // Extracting body after '@' sign for proper IDN decoding.
+        $body = explode('@', $body, 2)[0] . '@' . idn_to_utf8(explode('@', $body, 2)[1]);
+      }

I only wanted to have this part inside a helper.
Now you create a helper that is still not standing on its own. It contains varying special cases depending on the field it gets.

Plus the return type is mixed, with lying in annotation (you return array sometimes)... breaking autocompletion.

+++ b/src/MIME/MessageTrait.php
@@ -46,4 +31,37 @@ trait MessageTrait {
+   * @return string|null
...
+    return ($field == 'Cc' || $field == 'To') ? [$body] : $body;

I just wanted to have a method:

protected function decodeAddress($body) {
   if (strpos($body, '@') !== FALSE) {
     // Extracting body after '@' sign for proper IDN decoding.
     return explode('@', $body, 2)[0] . '@' . idn_to_utf8(explode('@', $body, 2)[1]);
   }
  return $body;
}

Still postponing for the overlap. We don't merge these kind of issues. The overlap would still have been minimal with the intention.

ModernMantra’s picture

Status: Postponed » Needs review
FileSize
5.4 KB

By some investigation, i made great mistake and mixed two things that are very related (thus providing here the fies that should be on another issue, that caused 'illusion' of overlap). Uploading 'clean' patch ready for review :). Interdiff too big... Setting needs review, IMHO it has almost nothing of overlap except idea :)

mbovan’s picture

Status: Needs review » Needs work
  1. +++ b/inmail_collect/src/Plugin/collect/Model/InmailMessage.php
    @@ -100,7 +100,7 @@ class InmailMessage extends ModelPluginBase implements ContainerFactoryPluginInt
    -      '#markup' => htmlentities($parsed_data->getTo()),
    +      '#markup' => $parsed_data->getTo(),
    
    +++ b/inmail_collect/src/Plugin/inmail/Handler/CollectHandler.php
    @@ -47,7 +47,7 @@ class CollectHandler extends HandlerBase {
    -    if (!$message->getReceivedDate() || !$message->getFrom() || !$message->getTo() || $message->getSubject() === NULL) {
    +    if (!$message->getReceivedDate() || !$message->getFrom() || $message->getSubject() === NULL) {
    
    +++ b/src/MIME/MessageInterface.php
    @@ -37,9 +37,8 @@ interface MessageInterface extends EntityInterface {
    -   * @return string|null
    -   *   The content of the 'To' header field, or null if that field does not
    -   *   exist.
    +   * @return array
    +   *   List of recipient addresses.
    

    Why these changes? I guess it's a leftover from #2808557: Make getTo() support multiple.

  2. +++ b/src/MIME/MessageInterface.php
    @@ -56,6 +55,17 @@ interface MessageInterface extends EntityInterface {
    +   * Returns the addresses of other recipients.
    ...
    +   *   The list of other recipients.
    

    Maybe: "The list of Cc recipients."

  3. +++ b/src/MIME/MessageInterface.php
    @@ -56,6 +55,17 @@ interface MessageInterface extends EntityInterface {
    +  public function getCC($decode = FALSE);
    

    getCc?

  4. +++ b/src/MIME/MessageTrait.php
    @@ -12,14 +12,7 @@ trait MessageTrait {
    -    if ($decode) {
    -      if (strpos($body, '@') !== FALSE) {
    -        // Extracting body after '@' sign for proper IDN decoding.
    -        $body = explode('@', $body, 2)[0] . '@' . idn_to_utf8(explode('@', $body, 2)[1]);
    -      }
    -      //@todo Properly parse Mail Address https://www.drupal.org/node/2800585
    -    }
    -    return $body;
    +    return ($decode) ? ($this->getDecodedAddress($body)) : ($body);
    
    @@ -27,13 +20,7 @@ trait MessageTrait {
    -    if ($decode) {
    -      if (strpos($body, '@') !== FALSE) {
    -        $body = explode('@', $body, 2)[0] . '@' . idn_to_utf8(explode('@', $body, 2)[1]);
    -      }
    -      //@todo Properly parse Mail Address https://www.drupal.org/node/2800585
    -    }
    -    return $body;
    +    return ($decode) ? ($this->getDecodedAddress($body)) : ($body);
    

    I guess these are changes related to #2808557: Make getTo() support multiple as well.

  5. +++ b/src/MIME/MessageTrait.php
    @@ -46,4 +33,30 @@ trait MessageTrait {
    +    return ($decode) ? ([$this->getDecodedAddress($body)]) : ([$body]);
    

    No need for parenthesis.

  6. +++ b/src/MIME/MessageTrait.php
    @@ -46,4 +33,30 @@ trait MessageTrait {
    +    if (strpos($body, '@') !== FALSE) {
    +      // Extracting body after '@' sign for proper IDN decoding.
    +      $body = explode('@', $body, 2)[0] . '@' . idn_to_utf8(explode('@', $body, 2)[1]);
    

    Do we want to support handling of multiple addresses here? Seems that getCc is calling it and then casting the result into a single-element array...

    Edit: This should be a followup in #11685573 #8.1.

  7. +++ b/src/MIME/MessageTrait.php
    @@ -46,4 +33,30 @@ trait MessageTrait {
    +    //@todo Properly parse Mail Address https://www.drupal.org/node/2800585.
    

    // @todo:

  8. +++ b/tests/src/Unit/MIME/MessageTest.php
    @@ -77,6 +77,17 @@ class MessageTest extends UnitTestCase {
    +   * Tests the other recipients getter.
    

    Tests Cc recipients.

  9. +++ b/tests/src/Unit/MIME/MessageTest.php
    @@ -77,6 +77,17 @@ class MessageTest extends UnitTestCase {
    +  public function testGetCC() {
    +    $message = new Message(new Header([['name' => 'Cc', 'body' => 'sun_is_shinning@example.com']]), 'I am a body');
    +    $cc_field = $message->getCC();
    +    $this->assertEquals('sun_is_shinning@example.com', reset($cc_field));
    +  }
    

    Can we add one more (IDN) case similarly to testGetTo()?

mbovan’s picture

Highly related issue.

ModernMantra’s picture

Status: Needs work » Needs review
FileSize
3.85 KB
3.89 KB
mbovan’s picture

Status: Needs review » Needs work

I still see lot of stuff mentioned ni #7.

miro_dietiker’s picture

Status: Needs work » Fixed
FileSize
6.81 KB
+++ b/src/MIME/MessageInterface.php
@@ -37,9 +37,8 @@ interface MessageInterface extends EntityInterface {
-   * @return string|null
-   *   The content of the 'To' header field, or null if that field does not
-   *   exist.
+   * @return array
+   *   List of recipient addresses.

Still unwanted change.. but i dropped it and committed the new method.

Helper method to convert...

Just "Converts..." - avoid unneeded words. It makes code / documentation lengthy without adding any value.

Big oops: idn_to_utf8 only exists if the php5_intl package is installed.
I added some conditionals so it doesn't die fatal.

And some more refactoring was needed, some extended checks now with the Unit tests.

miro_dietiker’s picture

Status: Needs work » Fixed

Committed this.

Completely missed that we need to use the method in the twig templates to display the mail.
Extended issue #2809597: Add Field "To" to Mail header display

Status: Fixed » Closed (fixed)

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