ValueError class not available in PHP < 8

[MarkMaldaba]

The MBString polyfill supports PHP >= 7.2 but it makes use of the ValueError exception class, which was only introduced in PHP 8.

In a lot of instances, there is a version check before it is used, and in these cases there is no problem.

However, there are three unguarded locations that refer to the class, which will result in fatal errors on PHP 7:

polyfill/src/Mbstring/Mbstring.php

Lines 837 to 841 in 731f6e1

	public static function mb_str_pad(string $string, int $length, string $pad_string = ' ', int $pad_type = \STR_PAD_RIGHT, ?string $encoding = null): string
	{
	if (!\in_array($pad_type, [\STR_PAD_RIGHT, \STR_PAD_LEFT, \STR_PAD_BOTH], true)) {
	throw new \ValueError('mb_str_pad(): Argument #4 ($pad_type) must be STR_PAD_LEFT, STR_PAD_RIGHT, or STR_PAD_BOTH');
	}

polyfill/src/Mbstring/Mbstring.php

Lines 849 to 851 in 731f6e1

	if (self::mb_strlen($pad_string, $encoding) <= 0) {
	throw new \ValueError('mb_str_pad(): Argument #3 ($pad_string) must be a non-empty string');
	}

polyfill/src/Mbstring/Mbstring.php

Lines 1040 to 1043 in 731f6e1

	// BC for PHP 7.3 and lower
	if (!$validEncoding) {
	throw new \ValueError(sprintf($errorFormat, $encoding));
	}

[derrabus]

Indeed, this looks like a bug. As a workaround, you can require the symfony/polyfill-php80 package in your project which should declare the missing ValueError class.

What shall we do here, @nicolas-grekas? The easy way would of course be to register the PHP 8.0 polyfill as a dependency of the mbstring polyfill.

[MarkMaldaba]

I'm aware that these new methods were only added in PHP 8 and therefore have always thrown ValueError exceptions, but I think if they had been present in older versions then they would have triggered errors/returned false like other similar mb_* functions.

Therefore, my recommendation would be to update the locations that are throwing ValueError to work the same as the older functions, which already have logic in place to handle this.

I guess, if you wanted to, you could use class_exists("ValueError") rather than a version check, so that if the shim was installed it would use it. However, I am very wary of an additional dependency being added and making the php8 polyfill a mandatory requirement.

Another thought: If using trigger_error()/return false seems a bit icky for functions that were never implemented in that way, you could just throw a standard Exception on older PHP versions.

tl;dr I'd prefer it if the solution didn't add an additional dependency, whatever it is.

[MarkMaldaba]

Another alternative is to define the specific polyfill for this exception class in the Mbstring polyfill codebase, that way behaviour is unchanged and there are no additional dependencies.

I'm not sure how that sits re: code duplication, but this looks like a pretty trivial class to shim, so perhaps it's not really an issue at this scale. It would certainly be the simplest/safest solution, imho.

[nicolas-grekas]

I'd say the polyfill shouldn't throw if it doesn't run on PHP 8. That'd also be the correct polyfilling behavior on PHP7.

[derrabus]

I'd say the polyfill shouldn't throw if it doesn't run on PHP 8. That'd also be the correct polyfilling behavior on PHP7.

So, oldschool trigger_error() and return false;?

[nicolas-grekas]

💯

[derrabus]

I can look into it tonight.

[derrabus]

#501