IntlBreakIterator::create methods cannot returns null

[VincentLanglet]

Hi,

I may be wrong but I feel like the stub is wrong about the fact those create method can returns null.

All the example I found are not checking null and when checking lot of non-locale values

\IntlBreakIterator::createWordInstance('azertyuiop')

I still get an IntlBreakIterator instance.

So I wonder if the stub is not wrong.

[devnexen]

Hi I ll have a look later but please target PHP-8.4 instead.

[VincentLanglet]

Hi I ll have a look later but please target PHP-8.4 instead.

I was looking at the right branch and found https://github.com/php/php-src/blob/master/CONTRIBUTING.md#git-commit-rules which was saying

PHP-8.3 | Is used to release the PHP 8.3.x series. This is a current stable version and is open for bugfixes only.

I update the PR and #21003 too then.

[iluuu1994]

Are you sure this should target 8.4? This class is not final, and this would be LSP breaking.

[VincentLanglet]

Are you sure this should target 8.4? This class is not final, and this would be LSP breaking.

Oh maybe I missunderstood this code ? I thought this was just a "stub" which has no real meaning for the php codebase except giving documentation for the php-doc website (or tool with auto generated doc like phpstorm).

Maybe you prefer I add phpdoc with a return tag instead ?

[devnexen]

So I looked, in theory it is possible for these to return null, in practice however it is pretty difficult to actually happen => bad/corrupted ICU database, broken ICU version, OOM ... but those are quite unlikely. Giving a bad locale does not work as you mentioned.

[iluuu1994]

Right, but even if this is unlikely, code overriding these methods and currently return nullable will break with an LSP error. That's not something that should happen in a patch.

[devnexen]

yes I was not gonna accept these changes

[iluuu1994]

@VincentLanglet To answer your question: Stubs are used for reflection, but also LSP (i.e. subtyping rules for parameters / return types). So, for methods that could be overridden, types should generally only change in master.

[VincentLanglet]

Ok, then even if it's unlikely it's "possible" and I should close the PR or should I "just" target master instead ?

Also, what about #21003@devnexen@iluuu1994 ? Should I target master or is it considered as a real fix ? I feel like the current type is wrong in the stub.

[iluuu1994]

If it's even theoretically possible that these functions will return a value, it should be reflected accordingly. Otherwise, it would need to be ensured that these values aren't returned, e.g. by throwing an exception instead.