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

Fix #65547: Default value for sunrise/sunset zenith still wrong #3088

Closed
wants to merge 1 commit into
base: master
from

Conversation

3 participants
@cmb69
Copy link
Contributor

cmb69 commented Feb 8, 2018

The value of the zenith angle to calculate sunrise and sunset times is
commonly defined as 90°50', and is "obtained by adding the average
apparent radius of the Sun (16') to the average amount of atmospheric
refraction at the horizon (34')", according to
http://aa.usno.navy.mil/faq/docs/RST_defs.php.

This value is also used for the Sunrise/Sunset Algorithm published in
the Almanac for Computers, 1990, see
https://web.archive.org/web/20161202180207/http://williams.best.vwh.net/sunrise_sunset_algorithm.htm.

Fix #65547: Default value for sunrise/sunset zenith still wrong
The value of the zenith angle to calculate sunrise and sunset times is
commonly defined as 90°50', and is "obtained by adding the average
apparent radius of the Sun (16') to the average amount of atmospheric
refraction at the horizon (34')", according to
http://aa.usno.navy.mil/faq/docs/RST_defs.php.

This value is also used for the Sunrise/Sunset Algorithm published in
the Almanac for Computers, 1990, see
https://web.archive.org/web/20161202180207/http://williams.best.vwh.net/sunrise_sunset_algorithm.htm.

@krakjoe krakjoe added the Bugfix label Feb 9, 2018

@krakjoe krakjoe requested a review from derickr Feb 12, 2018

@derickr
Copy link
Contributor

derickr left a comment

Although I probably agree that the new value is the more appropriate value, we can't change this as it's a BC break.

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Feb 13, 2018

@cmb69 target master please ...

@derickr

This comment has been minimized.

Copy link
Contributor

derickr commented Feb 13, 2018

Only if master is PHP 8 will I approve this.

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Feb 13, 2018

I was trying to be reasonable ... this is a bug fix in my opinion and is fine to target active branches, as a bug fix.

Targeting the next minor should give plenty of time for those people relying on broken internals to prepare themselves, shouldn't it ?

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Feb 13, 2018

Well, this is only about changing default ini settings (which are even PHP_INI_ALL), so whether we change it in a minor version or not, doesn't appear to be too much of an issue per se. However, I've just noticed that the respective value is hard-coded for date_sun_info(), so only changing the ini defaults would introduce an inconsistency.

I can fix date_sun_info() accordingly and target master, but that wouldn't make sense if the PR would have to wait for PHP 8, since it might takes years and the PR is likely to be forgotten till then.

@cmb69 cmb69 changed the base branch from PHP-7.1 to master Feb 14, 2018

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Feb 14, 2018

Changed base to master as requested.

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Mar 17, 2018

This appears to be too controversial for PHP 7, so I'm closing this PR. https://bugs.php.net/65547 is still open as a reminder to tackle the issue for PHP 8.

@cmb69 cmb69 closed this Mar 17, 2018

@cmb69 cmb69 deleted the cmb69:zenith-default branch Mar 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment