Use true return type for XML functions which always return true #9539
Conversation
| @@ -1406,6 +1415,7 @@ PHP_FUNCTION(xml_parser_set_option) | |||
| if (parser->toffset < 0) { | |||
| php_error_docref(NULL, E_WARNING, "tagstart ignored, because it is out of range"); | |||
| parser->toffset = 0; | |||
| /* TODO Should this return false? */ | |||
There was a problem hiding this comment.
I think this should throw a ValueError, but it might not be the best idea to change that now for BC reasons.
There was a problem hiding this comment.
It probably should in the long run, but currently it's an error condition which returns true which is somewhat awkward.
There was a problem hiding this comment.
Right! Given that we currently type as bool, returning false in this case appears to be the best solution for now.
There was a problem hiding this comment.
Personally, I'd probably stay with true + add this to the PHP 9.0 warning promotion RFC when the time comes, but I don't mind returning false.
There was a problem hiding this comment.
That's also fair, as it has been this behaviour for a while... PHP does have some oddities.
| @@ -1406,6 +1415,7 @@ PHP_FUNCTION(xml_parser_set_option) | |||
| if (parser->toffset < 0) { | |||
| php_error_docref(NULL, E_WARNING, "tagstart ignored, because it is out of range"); | |||
| parser->toffset = 0; | |||
| /* TODO Should this return false? */ | |||
There was a problem hiding this comment.
Personally, I'd probably stay with true + add this to the PHP 9.0 warning promotion RFC when the time comes, but I don't mind returning false.
I hope this can land for PHP-8.2. As I think those functions have only been returning true since PHP 8.0 and the false return I expect to be from ZPP failures.
Docs will probably need amending, but this might make more sense to do via the stubs generation script.