Fix #62548 #3083
Fix #62548 #3083
Conversation
The 3rd term in date-sunset and date-sunrise should be called 'azimuth', which is used in sunrise, sunset calculations to tell if the bearing of the sun from the observer is East or West of North.
|
According to Wikipedia:
And indeed, the altitude is calculated as complementary angle of the zenith (angle). The altitude is useful to calculate twilight times, or the exact sunrise/-set at a particular altitude. So bug 62548 is not a bug, but rather a documentation issue. |
| @@ -583,8 +583,8 @@ PHP_INI_BEGIN() | |||
| STD_PHP_INI_ENTRY("date.timezone", "", PHP_INI_ALL, OnUpdate_date_timezone, default_timezone, zend_date_globals, date_globals) | |||
| PHP_INI_ENTRY("date.default_latitude", DATE_DEFAULT_LATITUDE, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.default_longitude", DATE_DEFAULT_LONGITUDE, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.sunset_zenith", DATE_SUNSET_ZENITH, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.sunrise_zenith", DATE_SUNRISE_ZENITH, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.sunset_azimuth", DATE_SUNSET_AZIMUTH, PHP_INI_ALL, NULL) | |||
There was a problem hiding this comment.
Renaming parameters is fine, but these ini settings changes are a problem, they will cause a BC break.
(No idea about the patch in general.)
There was a problem hiding this comment.
Should we wait for 8.0, or leave it? As @cmb69 said, is more than a documentation improvement, than an actual bug fix.
There was a problem hiding this comment.
What I meant is that zenith is the correct term here (not azimuth), but the the parameter and the ini settings need better documentation.
There was a problem hiding this comment.
It's IMO not worth changing APIs for. If this gets changed, the ini settings for default lat/long should go, as well was as date_sunrise.
| @@ -583,8 +583,8 @@ PHP_INI_BEGIN() | |||
| STD_PHP_INI_ENTRY("date.timezone", "", PHP_INI_ALL, OnUpdate_date_timezone, default_timezone, zend_date_globals, date_globals) | |||
| PHP_INI_ENTRY("date.default_latitude", DATE_DEFAULT_LATITUDE, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.default_longitude", DATE_DEFAULT_LONGITUDE, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.sunset_zenith", DATE_SUNSET_ZENITH, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.sunrise_zenith", DATE_SUNRISE_ZENITH, PHP_INI_ALL, NULL) | |||
| PHP_INI_ENTRY("date.sunset_azimuth", DATE_SUNSET_AZIMUTH, PHP_INI_ALL, NULL) | |||
There was a problem hiding this comment.
It's IMO not worth changing APIs for. If this gets changed, the ini settings for default lat/long should go, as well was as date_sunrise.
|
Yet another reference that zenith is the appropriate term here: https://web.archive.org/web/20161202180207/http://williams.best.vwh.net/sunrise_sunset_algorithm.htm. |
|
@cmb69 I watched that video and thought I understood. Maybe this is why I'm a programmer and not a geographer 🤓😂 Feel free to close this and the bug report 😄 |
|
I've changed the bug report to documentation problem, and going to close this PR. Thanks nonetheless, Gabriel, since the PR has been a good start to improve the documentation, which is obviously necessary. |
|
Comment on behalf of cmb at php.net: Closing, since invalid. |
I even watch a video to confirm this https://www.youtube.com/watch?v=OR8EQ0DWpPw 😄
(PS: If this gets approved, how should I proceed to fix our documentation?)