Skip to content

Narrow bool return types to true when possible #2458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 7, 2023

Conversation

kocsismate
Copy link
Member

No description provided.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text in the return value must be amended for XML at least.

Also the XML functions did return false on an invalid resource type in PHP 7, but I think we consider this to be UB so no need to document.

@kamil-tekiela do you know since when the Mysqli functions only return true?

@kamil-tekiela
Copy link
Member

It was changed in php/php-src#5058, but as I understand standalone true is only possible since 8.2.

mysqli_debug always returned true but it could only be typed as bool due to lack of standalone true type.

@Girgias
Copy link
Member

Girgias commented May 5, 2023

It was changed in php/php-src#5058, but as I understand standalone true is only possible since 8.2.

mysqli_debug always returned true but it could only be typed as bool due to lack of standalone true type.

Okay that's just what I wanted to know if we need a changelog, which seems like we do for PHP 8.0 if it isn't already there. :)

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than openlog() everything LGTM

&return.success;
&return.true.always;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this used to return false in 8.0, so this needs some more investigation of what version this changed and why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@kocsismate kocsismate May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since it's a PHP 8.3 change (he only committed it to master in last August), I should revert this change :S

@kocsismate kocsismate merged commit f781803 into php:master May 7, 2023
@kocsismate kocsismate deleted the narrow-bool-return branch May 7, 2023 20:33
claudepache pushed a commit to claudepache/php-doc-en that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants