Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill

Interesting. I totally agree that we should add that.
Is there any way we could actually test that? I guess for example by removing the workaround in the token test?

We do part of this already in core/tests/bootstrap.php

// Set sane locale settings, to ensure consistent string, dates, times and
// numbers handling.
// @see \Drupal\Core\DrupalKernel::bootEnvironment()
setlocale(LC_ALL, 'C');

So that shouldn't be required. The way \Drupal\Component\Utility\Unicode::check() works is not nice. Why not make it lazy initialize? I.e if the status has not been set - set it correctly in getStatus(). /me looks at the way Symfony\Polyfill\Mbstring works and is jealous.

Oh hilarious... we're loading vendor/symfony/polyfill-mbstring/bootstrap.php cause Symfony uses it - so our Unicode could depend on that and we could remove the static completely.

Like so... no static and no unnecessary calls to Unicode::check() which now just does checking rather than any static initialisation.

Also Unicode::setStatus() is horrid - why that was added for unit tests I'll never know - we shoulda just used reflection. Ughs. Probably my fault :) #1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode

Oh and just for fun there is not D8 installation in the land where \Drupal\Component\Utility\Unicode::$status = \Drupal\Component\Utility\Unicode::STATUS_SINGLEBYTE apart from in our tests because function_exists('mb_strlen') will always return TRUE because of the polyfill :)

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -143,38 +143,33 @@ public static function setStatus($status) {
     if (!function_exists('mb_strlen')) {

Even more fun because this will never return FALSE we'll never recommend someone installs the mbstring extension!

Let's fix #9 and update the issue summary.

Fixing up UnicodeTest since there's no single byte code path to test anymore.

Missed a couple of things to clean up.

7 files changed, 48 insertions, 263 deletions. - less code yay!

Really nice work alex! I'll review it later properly.

+++ b/core/composer.json
@@ -18,6 +18,7 @@
         "symfony/polyfill-iconv": "~1.0",
+        "symfony/polyfill-mbstring": "~1.0",
         "symfony/yaml": "~2.8",

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -224,17 +219,7 @@ public static function encodingFromBOM($data) {
-    if (function_exists('iconv')) {
-      return @iconv($encoding, 'utf-8', $data);
-    }
-    elseif (function_exists('mb_convert_encoding')) {
-      return @mb_convert_encoding($data, 'utf-8', $encoding);
-    }
-    elseif (function_exists('recode_string')) {
-      return @recode_string($encoding . '..utf-8', $data);
-    }
-    // Cannot convert.
-    return FALSE;
+    return @iconv($encoding, 'utf-8', $data);

If the mb mb_convert_encoding() can do the same thing as iconv(), when why do we still have both libraries? Can we just use symfony/polyfill-mbstring instead of symfony/polyfill-iconv? Looks like this is the only usage on iconv() in core.

@mpdonadio well considering we already have a polyfill for iconv core is always going to be doing that code path. Hence the change I chose to make. And the mbstring string polyfill is dependent on the iconv polyfill cause it uses iconv too...

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -322,16 +292,7 @@ public static function strtoupper($text) {
    *   The string in lowercase.
    */
   public static function strtolower($text) {
...
+    return mb_strtolower($text);
   }

@@ -399,90 +360,7 @@ public static function ucwords($text) {
    *   The shortened string.
    */
   public static function substr($text, $start, $length = NULL) {

I am wondering whether we should deprecate most of those functions now.

@dawehner we totally could do that. On it.

Made Unicode::mb_substr() simpler too... the NULL check here is pointless - https://secure.php.net/manual/en/function.mb-substr.php

We should file a followup to replace all the calls to the deprecated functions.

Created the CR https://www.drupal.org/node/2850048 and filed the followup #2850046: Remove usages of \Drupal\Component\Utility\Unicode() functions

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -358,9 +367,12 @@ public static function ucwords($text) {
   public static function substr($text, $start, $length = NULL) {
-    return $length === NULL ? mb_substr($text, $start) : mb_substr($text, $start, $length);
+    return mb_substr($text, $start, $length);
   }

I'm wondering whether this is some weird performance optimization? I doubt it thought to be honest. Do we have an explicit test to ensure this actually works?

\Drupal\Tests\Component\Utility\UnicodeTest::testSubstr tests passing a NULL. I looked back at the original commit - #26688: Add mbstring support to Drupal no notes as to why - I just can't see it as a perf optimisation.

Fixed a typo and improved the deprecation message.

Rerolled the patch.

1. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -106,8 +99,15 @@ class Unicode {
   +  public static function getStatus()
   +  {
   +    switch (static::check()) {
   

   I would guess this file is not excluded from the CS checking, right?

2. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -279,15 +268,12 @@ public static function truncateBytes($string, $len) {
   +   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use
   
   @@ -298,18 +284,12 @@ public static function strlen($text) {
   +   * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use
   

   Nitpick: We need to point to 8.4.x now ...

+++ b/core/composer.json
@@ -17,6 +17,7 @@
+        "symfony/polyfill-mbstring": "~1.0",

we are requiring a new package but not any changes in composer.lock file?

@naveenvalecha that's because other things are already dependent on it. But also core/lib/Drupal/Component/Utility/composer.json should be update to have symfony/polyfill-mbstring too.

So here's a patch with proper deprecation notices for 8.6.0. The real patch with the conversions can be scripted by doing the following:

# Apply the smaller not tested issue patch https://www.drupal.org/project/drupal/issues/2849669

# Replace the occurrances including FQDN. Check core/lib/Drupal/Core/Mail/MailFormatHelper.php.
find ./core -type f -not -path "./core/node_modules/*" -not -path "./core/assets/*" -not -path "./core/misc" -exec sed -i -r 's/\\?(Drupal\\Component\\Utility\\)?Unicode::(substr|strlen|strtoupper|strtolower|strpos)/mb_\2/g;' {} \;

# Revert unnecessary changes
git co core/lib/Drupal/Component/Utility/Unicode.php
git co core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php

# Fix unused uses
composer run-script phpcbf -- --sniffs=Drupal.Classes.UnusedUseStatement,PSR2.Namespaces.NamespaceDeclaration

Not bad... just a few stragglers.

175 files changed, 401 insertions, 724 deletions. A lot less code and once we move to Drupal 9 less surface area to support too - 6 methods from Unicode are deprecated here.

Just mentioning because maybe someone more knowledgeable than me should close it: #2911497: Make \Drupal\Component\Utility\Unicode::truncateBytes more readable

Rerolled.

1. +++ b/core/includes/theme.maintenance.inc
   --- a/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
   +++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
   
   +++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
   +++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
   @@ -134,7 +134,7 @@ public function diff($from_lines, $to_lines) {
   
   @@ -134,7 +134,7 @@ public function diff($from_lines, $to_lines) {
       * Returns the whole line if it's small enough, or the MD5 hash otherwise.
       */
      protected function _line_hash($line) {
   -    if (Unicode::strlen($line) > $this::MAX_XREF_LENGTH) {
   +    if (mb_strlen($line) > $this::MAX_XREF_LENGTH) {
          return md5($line);
        }
        else {
   diff --git a/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   
   diff --git a/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   index eb7d9434c9..2c1f6c8fc1 100644
   
   index eb7d9434c9..2c1f6c8fc1 100644
   --- a/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   
   --- a/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   +++ b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   
   +++ b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   +++ b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
   @@ -64,7 +64,7 @@ public function addWords($words, $tag = '') {
   
   @@ -64,7 +64,7 @@ public function addWords($words, $tag = '') {
          }
          if ($word[0] == "\n") {
            $this->_flushLine($tag);
   -        $word = Unicode::substr($word, 1);
   +        $word = mb_substr($word, 1);
          }
          assert(!strstr($word, "\n"));
          $this->group .= $word;
   
   diff --git a/core/lib/Drupal/Component/Diff/WordLevelDiff.php b/core/lib/Drupal/Component/Diff/WordLevelDiff.php
   index a8c2f80f25..4f9ef06add 100644
   
   index a8c2f80f25..4f9ef06add 100644
   --- a/core/lib/Drupal/Component/Diff/WordLevelDiff.php
   
   --- a/core/lib/Drupal/Component/Diff/WordLevelDiff.php
   +++ b/core/lib/Drupal/Component/Diff/WordLevelDiff.php
   
   +++ b/core/lib/Drupal/Component/Diff/WordLevelDiff.php
   +++ b/core/lib/Drupal/Component/Diff/WordLevelDiff.php
   @@ -35,7 +35,7 @@ protected function _split($lines) {
   
   @@ -35,7 +35,7 @@ protected function _split($lines) {
            $words[] = "\n";
            $stripped[] = "\n";
          }
   -      if (Unicode::strlen($line) > $this::MAX_LINE_LENGTH) {
   +      if (mb_strlen($line) > $this::MAX_LINE_LENGTH) {
            $words[] = $line;
            $stripped[] = $line;
          }
   

   This doesn't remove the use statement

2. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -224,17 +222,7 @@ public static function encodingFromBOM($data) {
      public static function convertToUtf8($data, $encoding) {
   ...
      }
   

   While our test coverage for substr and others is quite good we just have 3 test cases for convertToUtf8. I'm wondering whether we should expand the test coverage for higher confidence.

3. +++ b/core/lib/Drupal/Component/Utility/composer.json
   @@ -7,7 +7,9 @@
   +    "symfony/polyfill-mbstring": "~1.0"
   

   Any reason to not already require 1.7 or so?

Thanks for the review @dawehner. #37.1 is caused because we ignore the Diff directory in core/phpcs.xml.dist so just removed them in the non-scripted patch.
Re. #37.2 well nothing is changing here. We are always using the polyfill anyway because if (function_exists('iconv')) { will always return TRUE. So i don't think extra testing buys us anything.
Re #37.3 well because it probably is not actually required? As a framework I think we should have as wide as possible dependencies.

+++ b/core/lib/Drupal/Component/Diff/Engine/DiffEngine.php
@@ -2,8 +2,6 @@
-use Drupal\Component\Utility\Unicode;

+++ b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
@@ -2,8 +2,6 @@
-use Drupal\Component\Utility\Unicode;

+++ b/core/lib/Drupal/Component/Diff/WordLevelDiff.php
@@ -3,7 +3,6 @@
-use Drupal\Component\Utility\Unicode;

Do these deletions break the dependency drupal/core-diff and drupal/core-utility? If so, let's remove it from composer.json.

Well we have a new dependency. But nice catch!

I can't see anything to change or nitpick on in #40, that looks super solid. Deleting code is the best! Awesome work.

The patch does not apply... rerolled...

error: patch failed: core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php:4
error: core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php: patch does not apply

The diff of #40 and #44 scripted patches is:

15c15
< index 36fb006fe6..d4b74fd3e4 100644
---
> index 83df3a2b54..5d78372081 100644
826c826
< index 37ed0e97a6..55f446e2b7 100644
---
> index fec1be9ad3..695f408198 100644
837c837
< @@ -989,8 +988,9 @@ public static function bootEnvironment($app_root = NULL) {
---
> @@ -996,8 +995,9 @@ public static function bootEnvironment($app_root = NULL) {
1279c1279
< index 2699fcfb69..7f9fcce037 100644
---
> index c200252b81..ccf5b32cc4 100644
1287c1287
<  use Drupal\content_translation\Tests\ContentTranslationUITestBase;
---
>  use Drupal\Tests\content_translation\Functional\ContentTranslationUITestBase;
1937c1937
< index 6ae439ec5a..bd598a8775 100644
---
> index 4e8ad91380..3bde95caa4 100644
1948c1948
< @@ -311,9 +310,9 @@ protected function preSaveNew(EntityStorageInterface $storage) {
---
> @@ -318,9 +317,9 @@ protected function preSaveNew(EntityStorageInterface $storage) {
2836c2836
< index dd261248a7..83b3164ee3 100644
---
> index da00c7f7d2..7992aea57a 100644
2847c2847
< @@ -348,7 +347,7 @@ public function supportsUri($uri) {
---
> @@ -355,7 +354,7 @@ public function supportsUri($uri) {
3855c3855
< index 86d06e35ee..8fa7fae168 100644
---
> index cf57770972..5edaf42afe 100644
4279c4279
< index b5317beb8c..165ba267a0 100644
---
> index 1f2917f559..316d063bf7 100644
4290c4290
< @@ -122,8 +121,8 @@ protected function processStubRow(Row $row) {
---
> @@ -168,8 +167,8 @@ protected function processStubRow(Row $row) {
4315c4315
< index 4cc4e4bb16..1c48dbf439 100644
---
> index a50f9cdab3..35689ee077 100644
4521c4521
< index 4929c7ecf7..503a651e18 100644
---
> index 24cd1162c5..32aac174c4 100644
5074c5074
< index 7eb6ecb70c..f46d1e8f11 100644
---
> index 1905f158fd..19b1f21037 100644

Can't interdiff because it is a reroll.

I'm wondering if we really want to deprecate Unicode::strlen() or leave them as wrappers, considering the class itself will be staying in core. With Unicode::strlen() we have a way to document that people shouldn't use strlen(), but without it we don't have a way to tell people to use mb_strlen() instead. Could we maybe just split the deprecations out to a separate issue?

I'd be against that (a) because ideally we won't load Unicode on many requests. (b) we have 100's of usages in core of strlen() already so it's not as if we're telling people to not use strlen(). They already have to decide what to use. And with this patch in D9 it'll be simpler. Just use PHP functions either mb_strlen() or strlen() depending on your use-case.

That's fair, back to RTBC.

1. +++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
   @@ -3,7 +3,6 @@
   -use Drupal\Component\Utility\Unicode;
   
   +++ b/core/lib/Drupal/Component/Utility/composer.json
   @@ -7,7 +7,9 @@
   +    "drupal/core-render": "^8.2",
   

   We have a circular reference here, core-render relies on core-utility - is that a problem? And if so do we have an existing issue to fix it (pre-existing in HEAD)?

2. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -375,7 +375,7 @@ protected function mapConditionOperator($operator) {
   +      // do not need the more expensive mb_strtoupper() because SQL statements are ASCII.
   

   >80 here, but pre-existing, so don't bother if not rerolling for something else

3. +++ b/core/modules/migrate/tests/src/Unit/process/CallbackTest.php
   @@ -59,4 +59,19 @@ public function providerCallbackExceptions() {
   +    return strtolower($string);
   

   uber-nit: I realise this is just a test, but should we use mb_strtolower here to set a good example?

4. +++ b/core/tests/Drupal/Tests/Component/Utility/UnicodeTest.php
   @@ -15,56 +15,11 @@
   +   * @expectedDeprecation \Drupal\Component\Utility\Unicode::setStatus() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. In Drupal 9 there will be no way to set the status and in Drupal 8 this ability has been removed because mb_*() functions are supplied using Symfony's polyfill. See https://www.drupal.org/node/2850048.
   

   Because these are component tests, should the composer.json of this component include the bridge in its require-dev?

Thanks for the review @larowlan re #48:

1. Certainly this is a preexisting issue. It only exists because SafeMarkup class - which is entirely deprecated - I've opened #2958358: Remove Drupal\Component\Utility's dependency on Drupal\Component\Render
2. Existing bug - tricky to fix as part of the script. Will supply a post script patch to :)
3. Fixed
4. I'd address test dependencies in the issue that decouples component testing from core. At the moment the composer.json for utility doesn't contain any dev dependencies. The issue for this is #2943856: [meta] Reorganize Components so they are testable in isolation

New patch also cleans up other comment flow issues found as part of checking out #48.2

The patch to commit is the huge one :)

Given that the only thing to fix in #48 was an uber-nit moving back to rtbc,

I can tell you with a high degree of certainty that Utility currently doesn't need dev dependencies: #2876669-47: Fix dependency version requirement declarations in components

However, when this patch lands, the travis tests will (or at least should) fail for the reasons in #48.4.

Still trying to figure out how to test for all this stuff in an easy way.

Actually, #51 is incorrect. I can't say with a high degree of certainty about Utility, because it was failing, so I commented it out of the travis test and never got back to it. :-)

Working on it now. https://github.com/paul-m/drupal_component_tester/commit/4004314d0e41a01...

Git managed to rebase this... let's see what the both thinks.

First, rewinding head to replay your work on top of it...
Applying: Do it
Applying: Fix mbstring check
Applying: Cleaning up the test
Applying: do it
Applying: Deprecate all the things
Applying: Fix deprecation message
Applying: Do it
Applying: Proper deprecations
Applying: Do it
Applying: Do it
Applying: Improve test
Applying: Fix Diff uses
Applying: New dependency
Applying: Do it
Applying: Script
Using index info to reconstruct a base tree...
A	core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
M	core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
M	core/modules/node/tests/src/Functional/NodeTypeTranslationTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/node/tests/src/Functional/NodeTypeTranslationTest.php
Auto-merging core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
Auto-merging core/modules/image/tests/src/Functional/ImageFieldDefaultImagesTest.php
Applying: post script

Yay, this looks great!

core/composer.json still has a reference to lib/Drupal/Component/Utility/Unicode.php in autoload->classmap. That should no longer be necessary after this. On the other hand, I think lib/Drupal/Component/Utility/Timer.php there has also been obsolete for a while so we can also fix that together in a follow-up. Leaving RTBC for now.

@tstoeckler there is an issue to remove those already - somewhere in the issue queue - it will be done as part of #1826054: [Meta] Expose Drupal Components outside of Drupal

OK, perfect then. RTBC++

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
--- a/core/lib/Drupal/Component/Utility/composer.json
+++ b/core/lib/Drupal/Component/Utility/composer.json

Based on the work from #52, I ended up discovering that drupal/core-utility depends on drupal/core-assertion, at least for running tests: https://github.com/paul-m/drupal_component_tester/blob/master/patch/fixe...

Leaving RTBC because we don't have a formal way to confirm this in the Drupalsphere (as opposed to the Travissphere).

One of these days we might get #2876669: Fix dependency version requirement declarations in components :-)

Re ran the script.

Rerolled again.

Rerolled.

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -123,12 +122,16 @@ public static function getStatus() {
   /**
@@ -143,38 +146,33 @@ public static function setStatus($status) {

@@ -143,38 +146,33 @@ public static function setStatus($status) {
    *   Otherwise, an empty string.
    */
   public static function check() {
+    // Set appropriate configuration.
+    mb_internal_encoding('utf-8');
+    mb_language('uni');

This seems like the wrong place for a lot of this - should we open another follow-up to factor it out elsewhere?

Is this really fixed?

Nope not yet. Here's the requested follow-up - good idea to move this somewhere else #2971452: Deprecate Unicode::check() and move the application set up stuff somewhere else.

Sorry I got interrupted, meant to commit/push the patch while also asking for the follow-up. I've committed/pushed the patch to 8.6.x now so marking this fixed again!

Just tagging #2876669: Fix dependency version requirement declarations in components because that needs a re-roll now.