Utils - Add multibyte and UTF-8 support

[paulbalandan]

Fixes #5297

[coveralls]

Coverage increased (+0.009%) to 92.828% when pulling d3637aa on paulbalandan:utils-multibyte into 3968d73 on FriendsOfPHP:master.

[SpacePossum]

Hi and thanks for the PR!

Currently we don't require the mb_ext , nor do we pollyfil it. Not sure what would be best way forward, making it a requirement, polyfil it using the SF package or adding a function_exist() check with a fallback with maybe a custom solution.
Thoughts @keradus , @julienfalque @kubawerlos ?

[keradus]

if we need mb_* support, i would require polyfill

[julienfalque]

I have no strong opinion about this. Requiring the polyfill would increase the tool's weight but the extension might not be widespread enough to avoid it. Anyway, whatever choice we make, it should be required in composer.json.

[SpacePossum]

I know this looks like scope creep, yet I feel like if we go this way we should take care of some other points in the code effected by this as well.

There are a few spots we do function_exists('mb_*. With the polyfil now added we should check if these will now always return true. We have to check if these have no unwanted side effects (for example if the polyfil does not behave exactly as the extension).

Places are:

• https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v3.2.1/src/Cache/Signature.php#L94
• https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v3.2.1/src/Fixer/Phpdoc/PhpdocAnnotationWithoutDotFixer.php#L123
• https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v3.2.1/src/Fixer/Alias/MbStrFunctionsFixer.php#L61

For the last one, MbStrFunctionsFixer, I think that one is already good as it checks if the functions are internal.

Btw. nice work on updating the regex, so close to RTM from me :)

[SpacePossum]

Thank you @paulbalandan.

[mynameisbogdan]

Any reason for why the check with function_exists was removed? This seems to be a BC break on PHP 7.3.

Uncaught ReflectionException: Function mb_str_split() does not exist in vendor/friendsofphp/php-cs-fixer/src/Fixer/Alias/MbStrFunctionsFixer.php:61

[julienfalque]

symfony/polyfill-mbstring was added as a dependency so this should not happen. How did you install PHP CS Fixer?

[mynameisbogdan]

Hello @julienfalque,

Well I'm removing polyfills when the extension is mandatory. I'm the only one who is doing this?

For example

{
    "minimum-stability": "dev",
    "prefer-stable": true,
    "require": {
        "php": "^7.3",
        "ext-ctype": "*",
        "ext-mbstring": "*",
        "friendsofphp/php-cs-fixer": "^3.3"
    },
    "replace": {
        "symfony/polyfill-ctype": "*",
        "symfony/polyfill-mbstring": "*",
        "symfony/polyfill-php56": "*",
        "symfony/polyfill-php70": "*",
        "symfony/polyfill-php71": "*",
        "symfony/polyfill-php72": "*",
        "symfony/polyfill-php73": "*"
    }
}

I know it's a weird configuration, but I'm just curious about the reasoning.

[SpacePossum]

If you remove a requirement from the application there is nothing much we can do to help you, I mean these are in there for the reason that they are required. If you share how you installed the app like Julien asked we might be able to help, but otherwise not sure what we can do here.

[mynameisbogdan]

@SpacePossum I already shared my composer.json. That should be enough.

[julienfalque]

I see, then the check is still required indeed. If you don't mind, please submit a PR to fix that.

[SpacePossum]

how is the check still required, is it not in the polyfil?

[julienfalque]

Without the polyfill, we would have to check whether the function exists even with ext-mbstring installed because the function was added in PHP 7.4. But the polyfill has a different behavior because it creates it regardless of the PHP version.

I'm actually not sure about this being a BC break. We can:

• consider using the polyfill is safe and we don't add the check back: people removing the polyfill do it at their own risk, but it means we rely on the polyfill's specific behavior, which one could considerer a bit risky
• or we consider the poolyfill's specific behavior a quirk and we need to add the check back because we still support some PHP versions before 7.4
• or we drop PHP 7.2 and 7.3 support, removing the problem entirely 😁️

I would go with last option but I'm fine with any.

[keradus]

extention evolves over time, they add new functionality. we don't version the extension, but we can version the polyfill. i would expect nobody messing up with PHP CS Fixer requirements.
If you have extension in place, but old one (eg for PHP 7.1), then the polyfill will add only missing functionalities.

(drop of PHP 7.2/7.3 support is short term solution, the same concern would reappear later)

[julienfalque]

So option 1: we keep current situation and don't add the check back?