[WIP] Fixes for mbstring with PHP 8 throwing exceptions instead of raising warnings

[Ayesh]

Related #218.

This is an attempt to workaround the PHP 8's behavior of throwing \TypeError and \ValueError exceptions on internal functions[1]. This is a rather massive change, so we will be taking a look at in bite-sizes.

Both Mbstring and its test classes get two new helper methods. They check the PHP version, and if the PHP version is 8.0 or later, it will throw an exception from the polyfill class, and the complementing helper method will expect the same type of error. Both helper methods accept the name of the new exception class as the last parameter. We currently use \TypeError, but we will need to manually review each test to make sure the type of errors they throw. In most cases, this will be a \ValueError to follow the semantics of the error message.

This PR is to get some feedback on if others would agree if this approach indeed makes sense. #219 already adds the new \ValueError class.

I considered to make the conditional expect/trigger helper methods into a trait, but traits are only available in PHP 5.4, but the polyfill needs to support PHP 5.3. However, because we need to split the repo to separate packages, this trait wouldn't be as handy as I thought it would be, so the helper methods are baked into the test and polyfill themselves.

[IonBazan]

LGTM but please fix code style to follow the guidelines. PHP-CS-Fixer should help you with that.

[Ayesh]

Thanks a lot @IonBazan - I have applied CS fixes on hunks this PR changes.

[stof]

I'd rather inline this than each place that creating this method. It will be more efficient, as OPCache can optimize the PHP_VERSION_ID check at compile time, and it will avoid passing class names as string.

And btw, this might also help in case the message needs to change in the PHP 8 exception.

[nicolas-grekas]

What about creating polyfills v2 instead? We would maintain two branches: v1 for <8 and v2 for >=8.

[stof]

What about creating polyfills v2 instead? We would maintain two branches: v1 for <8 and v2 for >=8.

seems like a good idea. Then the v2 can use native typehints in functions and let PHP throw the TypeError btw.

[derrabus]

What about creating polyfills v2 instead? We would maintain two branches: v1 for <8 and v2 for >=8.

seems like a good idea. Then the v2 can use native typehints in functions and let PHP throw the TypeError btw.

Creating a v2 with a higher php requirement would certainly ease future maintenance, but I wouldn‘t require php 8 just yet. But php 7.2 as a minimum would be fine imho.

[stof]

@derrabus the goal is not to abandon v1. the goal is to make it easier to implement polyfills that respect the warning to exception migration which happens in PHP 8.

[derrabus]

Yes, but if the consequence is that I need a different set of dependencies for php 7.4 and 8.0 in my projects, I don‘t think that we have a good solution.

[derrabus]

Also, it‘s way too soon to create a release that requires php 8. The ProxyManager experiment last year (a ProxyManager release with php 7.4 as min was tagged while 7.4 was still in development) showed this.

[jderusse]

Yes, but if the consequence is that I need a different set of dependencies for php 7.4 and 8.0 in my projects, I don‘t think that we have a good solution.

not necessary. If symfony/polyfill: 2 requires php: >=8 and symfony/polyfill: 1 conflicts with php: >=8, then you can requires symfony/polyfill: 1 || 2 in your application and let composer choosing the right version of the polyfill.

I think following major versions of PHP make sens for polyfills.

[derrabus]

then you can requires symfony/polyfill: 1 || 2 in your application and let composer choosing the right version of the polyfill.

That is my point exactly. Composer has to chose a different version depending on my php version. The consequence is that I cannot produce a build of my application that can be tested on php 7.4 and 8.0 alike. This is the kind of stuff that makes migrations painful. Please let‘s not do this.

[nicolas-grekas]

I agree with @derrabus, relying on composer for this is risky practice, better avoid it.

[nicolas-grekas]

Closing in favor of #316