Skip to content
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

Update return values for floor() function #3007

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Update return values for floor() function #3007

merged 1 commit into from
Dec 12, 2023

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Dec 11, 2023

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.

Please remove the changelogs changes, we do not document the behaviour of passing invalid types to functions as this was never standard.

Comment on lines 42 to 64
<refsect1 role="changelog">
&reftitle.changelog;
<informaltable>
<tgroup cols="2">
<thead>
<row>
<entry>&Version;</entry>
<entry>&Description;</entry>
</row>
</thead>
<tbody>
<row>
<entry>8.0.0</entry>
<entry>
<parameter>num</parameter> no longer accepts internal objects which support
numeric conversion. Previous to this version, this function returns &false;
in case of an error and an <constant>E_WARNING</constant> error is emitted.
</entry>
</row>
</tbody>
</tgroup>
</informaltable>
</refsect1>
Copy link
Member

Choose a reason for hiding this comment

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

This is invalid, it never supported internal objects.

@@ -57,7 +57,8 @@
<entry>8.0.0</entry>
<entry>
<parameter>num</parameter> no longer accepts internal objects which support
numeric conversion.
numeric conversion. Previous to this version, this function returns &false;
in case of an error and an <constant>E_WARNING</constant> error is emitted.
Copy link
Member

Choose a reason for hiding this comment

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

An E_WARNING is not always emitted, an E_NOTICE is emitted for objects see: https://3v4l.org/SGfsd a warning is only emitted for non-numeric strings as far as I can tell.

And other types such as arrays do not emit any diagnostic at all: https://3v4l.org/FCHmh

Moreover, I consider those to be ZPP/Type failures and something we do not document.

@phansys phansys changed the title Normalize the changelog about how errors were handled previous to PHP 8 in math functions Update return values for floor() function Dec 12, 2023
@phansys
Copy link
Contributor Author

phansys commented Dec 12, 2023

Please remove the changelogs changes, we do not document the behaviour of passing invalid types to functions as this was never standard.

Thanks for the feedback.
Based on your insights, I reverted all the changes except for floor().

@phansys phansys requested a review from Girgias December 12, 2023 13:38
@Girgias Girgias merged commit 69194f0 into php:master Dec 12, 2023
2 checks passed
@Girgias
Copy link
Member

Girgias commented Dec 12, 2023

Thank you!

@phansys phansys deleted the math branch December 12, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants