Fixed bug #73239 (DateTime shows strange error message with invalid timezone) #8594

derickr · 2022-05-20T14:01:54Z

No description provided.

cmb69

Thank you! Generally good, but I'm not sure whether we should fix that long standing behavior in a revision.

iluuu1994

@cmb69 I don't think any project other than php-src should depend on specific error messages, so breakage here is unlikely. But since this is not critical master would be fine too.

Edit: Some tests need to be fixed of course. @derickr Now you have the chance to try --bless 😉

nikic · 2022-05-21T09:43:36Z

ext/date/tests/bug73239.phpt

+?>
+--EXPECTF--
+Warning: ini_set(): Invalid date.timezone value 'dummy' in %sbug73239.php on line %d
+DateTime::__construct(): Invalid date.timezone value 'dummy', we selected the timezone 'UTC' for now.


I think you might have mixed things up? The "we selected the timezone 'UTC' for now" should be on the ini_set() error message (where it makes sense) and not the DateTime::__construct() message (where it doesn't, because of the exception).

The patch, as is, removes both "we selected the timezone 'UTC' for now" messages; only that test expection isn't in line with that. I agree though, that the message raised from PHP_INI_MH(OnUpdate_date_timezone) might better stay as is (well, not really the wording; I'd go with passive voice instead).

No, I did not mix this up. The ini_set() warning is correct. At that state, PHP never fell back to UTC, it just shown the warning to hint that you messed up.

The warning for DateTime was correct as in the test, the code is wrong. It should very much tell you there that there was an actual fallback to UTC. And I have updated the wording.

I don't want to change the behaviour here.

Well, but throwing an exception with the message "DateTime::__construct(): UTC was used as timezone, because the date.timezone value 'dummy' is invalid" doesn't make sense. If UTC was used, there is no reason to throw an exception. And when an exception is thrown, that UTC is irrelevant. Or do I miss something?

Oh yes, of course. That's why I originally had removed that part of the sentence. I'll fix that back then. Need more 🍵 .

It isn't actually that simple. The location where this warning message is thrown, is in guess_timezone:

if (!timelib_timezone_id_is_valid(DATEG(default_timezone), tzdb)) { php_error_docref(NULL, E_WARNING, "UTC was used as timezone, because the date.timezone value '%s' is invalid", DATEG(default_timezone)); return "UTC"; }

Where the warning is correct. This same guess_timezone function is also called in date_initialize (via get_timezone_info()). get_timezone_info() is used plenty of times, where the warning message should be shown.

However, get_timezone_info() is also called as part of the DateTime constructor, where then the exception message is indeed silly, although it should refuse to construct the DateTime object as it currently does. Instead the exception ought to say the same as the warning from ini_set(). However, having a flag on the function to switch between two error messages is perhaps silly too, so I don't have a real good answer here.

nikic · 2022-05-27T10:15:41Z

I think what we should be doing here is to fail the ini update if the timezone is invalid (i.e. keep the previous valid value). This way we cannot hit this error condition in guess_timezone at all and the problem is nicely resolved.

derickr · 2022-05-27T12:48:51Z

OK, with this latest commit, the warning now only shows up when setting a wrong timezone through date.timezone.

nikic · 2022-05-27T14:05:07Z

ext/date/php_date.c

@@ -497,21 +497,19 @@ timelib_tzinfo *php_date_parse_tzfile_wrapper(const char *formal_tzname, const t
 /* {{{ static PHP_INI_MH(OnUpdate_date_timezone) */
 static PHP_INI_MH(OnUpdate_date_timezone)
 {
-	if (OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage) == FAILURE) {
+	DATEG(timezone_valid) = 0;


I don't think this assignment is needed anymore?

And possibly the entire timezone_valid flag?

Perhaps not, but I didn't want to risk just removing it in case there is a weird edge case that I didn't think of, and it is used in other places.

As far as I can tell this is the one place that would assign zero to it, others assign 1 or check for 1. We should probably retarget this change to master though, as there's some minor changes in behavior (as seen in tests).

Girgias · 2022-05-30T12:10:51Z

ext/date/php_date.c

+	if (OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage) == FAILURE) {
+		return FAILURE;
 	}

 	return SUCCESS;


Suggested change

if (OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage) == FAILURE) {

return FAILURE;

}

return SUCCESS;

return OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);

Maybe?

derickr · 2022-05-30T12:12:00Z

@nikic You OK with this now?

nikic

Yeah, looks good to me.

nikic · 2022-05-30T12:43:45Z

ext/date/php_date.c

@@ -478,19 +477,13 @@ timelib_tzinfo *php_date_parse_tzfile_wrapper(const char *formal_tzname, const t
 /* {{{ static PHP_INI_MH(OnUpdate_date_timezone) */
 static PHP_INI_MH(OnUpdate_date_timezone)
 {
-	if (OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage) == FAILURE) {
+	if (new_value && ZSTR_VAL(new_value) && !timelib_timezone_id_is_valid(ZSTR_VAL(new_value), DATE_TIMEZONEDB)) {
+		php_error_docref(NULL, E_WARNING, "Invalid date.timezone value '%s', using 'UTC' instead", ZSTR_VAL(new_value));


Strictly speaking, this isn't using UTC but whatever the old value is. Maybe we should print the old value instead?

…imezone)

derickr added the Extension: date label May 20, 2022

derickr requested review from iluuu1994 and cmb69 May 20, 2022 14:01

derickr self-assigned this May 20, 2022

cmb69 reviewed May 20, 2022

View reviewed changes

iluuu1994 approved these changes May 20, 2022

View reviewed changes

nikic requested changes May 21, 2022

View reviewed changes

derickr force-pushed the bug73239 branch from 6a93be0 to d5ff312 Compare May 27, 2022 09:21

derickr force-pushed the bug73239 branch 2 times, most recently from f7cee44 to 438c798 Compare May 27, 2022 13:50

nikic reviewed May 27, 2022

View reviewed changes

derickr force-pushed the bug73239 branch from 438c798 to 8d13d3b Compare May 29, 2022 16:21

derickr changed the base branch from PHP-8.0 to master May 29, 2022 16:21

Girgias approved these changes May 30, 2022

View reviewed changes

nikic approved these changes May 30, 2022

View reviewed changes

derickr added 5 commits June 2, 2022 09:24

Fixed bug #73239 (DateTime shows strange error message with invalid t…

b3c6b54

…imezone)

Update wording of warning message, and reinstate 'UTC' fallback for ctor

e0d2129

Only warn when an incorrect timezone is set through 'date.timezone'

6770158

Remove no longer used 'timezone_valid' flag

c85fcbc

Improve error message, and add additional test.

63aa5a4

derickr force-pushed the bug73239 branch from 590d7fe to 63aa5a4 Compare June 2, 2022 08:24

derickr merged commit 171ebb1 into php:master Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bug #73239 (DateTime shows strange error message with invalid timezone) #8594

Fixed bug #73239 (DateTime shows strange error message with invalid timezone) #8594

derickr commented May 20, 2022

cmb69 left a comment

iluuu1994 left a comment •

edited

Loading

nikic May 21, 2022

cmb69 May 23, 2022

derickr May 27, 2022

cmb69 May 27, 2022

derickr May 27, 2022

derickr May 27, 2022 •

edited

Loading

nikic commented May 27, 2022

derickr commented May 27, 2022

nikic May 27, 2022

nikic May 27, 2022

derickr May 27, 2022 •

edited

Loading

nikic May 27, 2022

Girgias May 30, 2022

derickr commented May 30, 2022

nikic left a comment

nikic May 30, 2022

Fixed bug #73239 (DateTime shows strange error message with invalid timezone) #8594

Fixed bug #73239 (DateTime shows strange error message with invalid timezone) #8594

Conversation

derickr commented May 20, 2022

cmb69 left a comment

Choose a reason for hiding this comment

iluuu1994 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickr May 27, 2022 • edited Loading

Choose a reason for hiding this comment

nikic commented May 27, 2022

derickr commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickr May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickr commented May 30, 2022

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 left a comment •

edited

Loading

derickr May 27, 2022 •

edited

Loading

derickr May 27, 2022 •

edited

Loading