Unicode::mimeHeaderDecode() doesn't support lowercased encoding

Patch applies cleanly to 9.1.x.

The patch is vaild to 9.1.x, RED/GREEN as expected. Per RFC 2047, the changes are good. And the patch is good to go to me.

The IS looks fine and is up to date, yay! I didn't see any questions unanswered in the issue.

Looking at the patch , I found a few items.

1. +++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
   @@ -75,6 +75,39 @@ public function testMimeHeader($value, $encoded) {
   +   * @see testMimeHeaderDecode()
   

   Is this necessary? I don't add an @see in the data providers that I have written.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
   @@ -75,6 +75,39 @@ public function testMimeHeader($value, $encoded) {
   +    return array(
   +      // Upper cased base64 encoding.
   

   Needs to use [] syntax. (Shows how old this patch is).

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -75,6 +75,39 @@ public function testMimeHeader($value, $encoded) {
+      // Upper cased base64 encoding.
+      array('tést.txt', '=?utf-8?B?dMOpc3QudHh0?='),
+      // Upper cased quoted-printable encoding.
+      array('tést.txt', '=?UTF-8?Q?t=C3=A9st.txt?='),
+      // Lower cased base64 encoding.
+      array('tést.txt', '=?utf-8?b?dMOpc3QudHh0?='),
+      // Lower cased quoted-printable encoding.
+      array('tést.txt', '=?UTF-8?q?t=C3=A9st.txt?='),
+      // Simple ASCII characters.
+      array('ASCII', 'ASCII'),

Thanks, @quietone for reviewing. Addressing, #12 meanwhile, transferred comments into keys.

Using the comments as key seems to be the trend, and it is helpful.

One more thing.

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -26,6 +26,32 @@ public function testMimeHeader($value, $encoded) {
+      'Upper cased base64 encoding' => ['tést.txt', '=?utf-8?B?dMOpc3QudHh0?='],
+      'Upper cased quoted-printable encoding' => ['tést.txt', '=?UTF-8?Q?t=C3=A9st.txt?='],

Since some of the line are greater than 80 chars let's format them like this

      'Upper cased base64 encoding' => [
        'tést.txt',
        '=?utf-8?B?dMOpc3QudHh0?=',
      ],

which is much easier to read.

Thanks! @quietone.

These changes make it header to read than it was and in other places we've kept the single line. Why are we doing this here?

-- from https://www.drupal.org/project/drupal/issues/2875279#comment-13781707

alexpott seems prefer #13

I noticed it. The rule Drupal.Arrays.Array.LongLineDeclaration has not been applied in core yet. One liners seem compact and easier to read. Well, addressing.

@quietone:
Thanks. I doubt that the mix of array (multiline vs single line) will be liked by committers.

All are multilines now.

Curious why ASCII is used twice.

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -44,17 +44,26 @@ public function testMimeHeaderDecode($value, $encoded) {
+        'ASCII',
+        'ASCII',

The only other thing is wording. IMO, it would be cleaner to use:

Uppercase instead of Upper cased

and

Lowercase instead of Lower cased

Otherwise Looks RTBC unless any of the items mentioned above should be changed.

1) Patch still applies cleanly.

2) Tests pass.

3) Includes new test to test the issue.

4) Patch addresses issue from issue summary.

5) Feedback from previous comments has been addressed.

6) Title and issue summary are clear.

Agree with @Kristen Pol about Upper/Lower cased => Uppercase/Lowercase.

Also the new test case seems to be inserted between an existing test case and its data provider; would be better to keep them together and insert the new test after providerTestMimeHeader.

Is it also worth refactoring testMimeHeader() to test the encoding side only? There is some duplication now as both methods test the decoding method with the same set of data.

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -26,6 +26,47 @@ public function testMimeHeader($value, $encoded) {
+        'ASCII',
+        'ASCII',

This would fit better with the other test cases if this was "test.txt" I think, I realise this is copied from the other test but maybe worth changing there too?

@Kristen Pol, @longwave, thanks for reviewing.

Curious why ASCII is used twice.

See \Drupal\Component\Utility\Unicode::mimeHeaderEncode();

For headers consist of ASCII chars, it returns as the original. Or see http://www.rfc-editor.org/rfc/rfc2047.txt for more information.

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -26,6 +26,47 @@ public function testMimeHeader($value, $encoded) {
+        ],
+      'Upper cased quoted-printable encoding' => [

Addressing #18, #19 and fixing the wrong indentation above.

Random fail. Requeued.

> Is it also worth refactoring testMimeHeader() to test the encoding side only?

Thinking again this is out of scope, and an extra two unit test passes don't take any time, so let's leave this alone.

All other review points addressed so RTBC assuming tests pass this time!

Thanks @longwave for RTBC-ing!

Thinking again this is out of scope, and an extra two unit test passes don't take any time, so let's leave this alone.

Sorry missed this.

testMimeHeader() covers both ::mimeHeaderDecode, ::mimeHeaderEncode previously, now testMimeHeaderDecode() added to test ::mimeHeaderDecode only. Renaming and refactoring testMimeHeader() to test ::mimeHeaderEncode only makes sense and it's in scope to me.

Addressing, but stay RTBC.

Committed 1c7065d and pushed to 9.1.x. Thanks!

This looks backportable to 9.0.x and 8.9.x, but we're in a freeze so just moving there for now.

@catch, thanks for committing!

The patch in #24 is vailid to 9.0.x. Rerolling one for 8,9.x

$ cat core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php.rej
diff a/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php    (rejected hunks)
@@ -15,15 +15,13 @@
 class UnicodeTest extends TestCase {
 
   /**
-   * Tests multibyte encoding and decoding.
+   * Tests multibyte encoding.
    *
    * @dataProvider providerTestMimeHeader
    * @covers ::mimeHeaderEncode
-   * @covers ::mimeHeaderDecode
    */
-  public function testMimeHeader($value, $encoded) {
+  public function testMimeHeaderEncode($value, $encoded) {
     $this->assertEquals($encoded, Unicode::mimeHeaderEncode($value));
-    $this->assertEquals($value, Unicode::mimeHeaderDecode($encoded));
   }
 
   /**

+++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
@@ -23,15 +23,13 @@ public function testSetStatus() {
    * @dataProvider providerTestMimeHeader

@@ -44,9 +42,49 @@ public function testMimeHeader($value, $encoded) {
+   * @dataProvider providerTestMimeHeaderDecode

To pair with providerTestMimeHeaderDecode(), would be better renaming providerTestMimeHeader() to providerTestMimeHeaderEncode(), do it in a separate issue as it got committed to 9.1.x already or ignore it -- it does not matter keeping it untouched.

Tagging "Needs followup" please if necessary.

Backported the original patch to 8.9.x
Committed 7f3f6de and pushed to 8.9.x.