Skip to content

Throw ValueError when 1 is provided to the second argument of log()#19370

Open
alexandre-daubois wants to merge 1 commit intophp:masterfrom
alexandre-daubois:log-base-1
Open

Throw ValueError when 1 is provided to the second argument of log()#19370
alexandre-daubois wants to merge 1 commit intophp:masterfrom
alexandre-daubois:log-base-1

Conversation

@alexandre-daubois
Copy link
Copy Markdown
Member

@alexandre-daubois alexandre-daubois commented Aug 5, 2025

This helps with early error detection and explicit intent, instead of having a potential NaN propagating.

Copy link
Copy Markdown
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Seems like a BC break to me so this might need at least some discussion. Not sure if NaN is worse than exception.

Comment on lines +670 to +671
if (base <= 0.0 || base == 1.0) {
zend_argument_value_error(2, "must not be 1 or less than or equal to 0");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would split it to two independent messages if we decide to do it.

@alexandre-daubois
Copy link
Copy Markdown
Member Author

I understand, I would also be fine throwing a deprecation for now

@alexandre-daubois
Copy link
Copy Markdown
Member Author

I removed this proposition from https://wiki.php.net/rfc/deprecations_php_8_6, rebased and added an UPGRADGING entry.

@Girgias
Copy link
Copy Markdown
Member

Girgias commented Apr 8, 2026

Not sure if this falls into the same category. Returning NAN is technically standard behaviour in some other PLs. Might be a good idea to see what other PLs do with their log function, or just keep in on 8.6 deprecation RFC.

@alexandre-daubois
Copy link
Copy Markdown
Member Author

Alright, let's wait for new inputs and I'll add it back to the RFC if there's no news in the coming weeks

@TimWolla
Copy link
Copy Markdown
Member

TimWolla commented Apr 8, 2026

instead of having a potential NaN propagating.

NaN explicitly exists so that one does not need to check each intermediate value. Specifically with floats experiencing implicit rounding, it's not always easy or cheap to avoid specific "malformed" inputs in a chain of operations (vs some class of inputs, such as "all negative numbers" that are reasonable to check for).

Trying to diverge from principle goes against the intentional design of IEEE 754 and I would suggest this not be done (and not even put to vote).

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.

4 participants