Skip to content

Fix GH-18481: date_sunrise check sun rise with offset if is finite/is… #18484

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix GH-18481: date_sunrise check sun rise with offset if is finite/is… #18484

wants to merge 2 commits into from
+22 −1

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented May 2, 2025

… nan

@devnexen devnexen marked this pull request as ready for review May 2, 2025 11:57
@devnexen devnexen requested a review from derickr as a code owner May 2, 2025 11:57
@devnexen devnexen linked an issue May 2, 2025 that may be closed by this pull request
Unexpected nan value ext/date/php_date.c #18481
Closed
nielsdos
nielsdos approved these changes May 3, 2025
View reviewed changes
ext/date/php_date.c Outdated
@@ -5346,7 +5346,7 @@ static void php_do_date_sunrise_sunset(INTERNAL_FUNCTION_PARAMETERS, bool calc_s
if (N > 24 || N < 0) {
N -= floor(N / 24) * 24;
}
if (N > 24 || N < 0) {
if (N > 24 || N < 0 || !zend_finite(N)) {
Copy link
Member

Choose a reason for hiding this comment

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

It's also possible to do it like this: if (!(N <= 24 && N >= 0)) {, but yours works too.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure I m gonna try your simpler approach.

Copy link
Member

Choose a reason for hiding this comment

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

I think you copied the condition incorrectly

Copy link
Member Author

Choose a reason for hiding this comment

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

yes..

ext/date/tests/gh18481.phpt Outdated
@@ -0,0 +1,21 @@
--TEST--
GH-18481 date_sunrise with utcOffset as INF
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GH-18481 date_sunrise with utcOffset as INF
GH-18481 (date_sunrise with utcOffset as INF)

@devnexen devnexen closed this in 0227d96 May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

@derickr derickr Awaiting requested review from derickr derickr is a code owner

Assignees
No one assigned
Labels
Extension: date
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected nan value ext/date/php_date.c
2 participants
@devnexen @nielsdos