Implement DateTime Exceptions RFC #10366

derickr · 2023-01-18T11:15:18Z

The feedback that I am mostly interesting in is whether I have not inadvertently made non-OO APIs throw exceptions (in addition to what was already happening), and whether I haven't typoed the specific classes to go with each warning/error message, and whether I missed any test cases. I think I got them all :-)

ext/date/php_date.c

Girgias · 2023-01-20T17:47:59Z

ext/date/php_date.c

+	if (ce->type == ZEND_INTERNAL_CLASS) {
+		zend_throw_error(date_ce_date_object_error, "The %s object has not been correctly initialized by its constructor by calling parent::__construct()", ZSTR_VAL(ce->name));
+	} else {


I'm confused here, doesn't this case mean that the actual DateTime object is not initialized? As such the problem is not fixed by calling a parent constructor.

Can this case only happen when using reflection to instantiate an object without calling the constructor? In that case, is there a test for it?

Slightly different, maybe easier-to-read alternative for the message:

Object of type %s has not been initialized by calling parent::__construct() in its constructor.

(Object of type wording is used all over php-src)

I'm confused here, doesn't this case mean that the actual DateTime object is not initialized? As such the problem is not fixed by calling a parent constructor.

Calling the parent constructor (DateTime::__construct()) will make it initialised correctly.

Can this case only happen when using reflection to instantiate an object without calling the constructor?

No, also with inheritance.

In that case, is there a test for it?

Yes, ext/date/tests/bug-gh8471.phpt.

I will make the change in text that @kocsismate suggests.

Girgias · 2023-01-20T17:50:50Z

ext/date/php_date.c

+	zend_replace_error_handling(EH_THROW, date_ce_date_malformed_string_exception, &zeh);
+	if (!php_date_modify(object, modify, modify_len)) {
+		zend_restore_error_handling(&zeh);
+		RETURN_FALSE;
+	}


I don't really like this trick of replacing the error handler, as the exception message is slightly off.
The only way to really fix this would be to add a php_date_modify_ex() function that has a boolean argument which dictates if it throws or warns.

Also:

Suggested change

zend_replace_error_handling(EH_THROW, date_ce_date_malformed_string_exception, &zeh);

if (!php_date_modify(object, modify, modify_len)) {

zend_restore_error_handling(&zeh);

RETURN_FALSE;

}

zend_replace_error_handling(EH_THROW, date_ce_date_malformed_string_exception, &zeh);

if (!php_date_modify(object, modify, modify_len)) {

zend_restore_error_handling(&zeh);

RETURN_THROWS();

}

I do like the error replacing handler here. It saves having to do if/else for every single time where a warning/exception has to be created. And I disapprove of flags that change the behaviour of return types/error handling depending on a flag. So it is staying.

Fixing the RETURN_THROWS() though.

ext/date/php_date.c

Girgias · 2023-01-20T17:58:13Z

ext/date/php_date.c

+		RETVAL_FALSE;
+		goto cleanup;
+	}
+
+	if (time->have_date || time->have_time || time->have_zone) {
+		zend_throw_error(date_ce_date_malformed_interval_string_exception, "String '%s' contains non-relative elements", ZSTR_VAL(time_str));
+		RETVAL_FALSE;


The REVAL_FALSE; should be unnecessary here as you are throwing an error.

RETURN_THROWS should be used here and above as well

Also, we have the zend_argument_error function which serves as the quasi-standard way to handle argument related errors. PS. exact string values are usually not exposed in error messages.

zend_argument_error doesn't allow for a different exception type, and RETURN_THROWS does not have a non-return version. With RETURN_THROWS() the function would immediately exit, leaking memory.

I'll remove the RETVAL_FALSE though!

This PR also does not intend to make exception messages less useful.

Girgias · 2023-01-20T18:00:00Z

ext/date/tests/DateTimeZone_compare_basic1.phpt

+Fatal error: Uncaught DateException: Cannot compare two different kinds of DateTimeZone objects in %s
+Stack trace:
+#0 {main}
+  thrown in %s


Wrap this call in a try/catch and echo $e::class, ': ', $e->getMessage(), PHP_EOL;?

Fixed - it also missed the last assertion in this test due to the fatal error.

ext/date/tests/DateTimeZone_serialize_errors.phpt

ext/date/tests/DateTime_modify_invalid_format.phpt

TimWolla

I did one first pass across the tests + stub, not looking at the C code yet. I've also not yet verified that all the exception classes match the RFC.

ext/date/tests/date_interval_non_relative_warning.phpt

ext/date/tests/date_interval_create_from_date_string_nullparam.phpt

ext/date/tests/bug78139.phpt

ext/date/tests/DateTime_wakeup_exception.phpt

ext/date/tests/DateTime_set_state_exception.phpt

ext/date/tests/DatePeriod_wrong_arguments.phpt

ext/date/tests/DatePeriod_uninitialised_exceptions.phpt

ext/date/tests/DatePeriod_set_state_exception.phpt

ext/date/tests/DateInterval_set_state_exception.phpt

ext/date/php_date.stub.php

kocsismate · 2023-01-20T21:04:35Z

ext/date/php_date.c

@@ -3733,7 +3830,7 @@ PHP_METHOD(DateTimeZone, __wakeup)
 	myht = Z_OBJPROP_P(object);

 	if (!php_date_timezone_initialize_from_hash(&return_value, &tzobj, myht)) {
-		zend_throw_error(NULL, "Timezone initialization failed");
+		zend_throw_error(NULL, "Invalid serialization data for DateTimeZone object");


now you don't want to specify the exact class name? (the same above)

__set_state and __wakeup are static methods, so how can it be any other class?

kocsismate · 2023-01-20T21:06:58Z

ext/date/php_date.c

+		RETVAL_FALSE;
+		goto cleanup;
+	}
+
+	if (time->have_date || time->have_time || time->have_zone) {
+		zend_throw_error(date_ce_date_malformed_interval_string_exception, "String '%s' contains non-relative elements", ZSTR_VAL(time_str));
+		RETVAL_FALSE;


RETURN_THROWS should be used here and above as well

kocsismate · 2023-01-20T21:09:51Z

ext/date/php_date.c

+		RETVAL_FALSE;
+		goto cleanup;
+	}
+
+	if (time->have_date || time->have_time || time->have_zone) {
+		zend_throw_error(date_ce_date_malformed_interval_string_exception, "String '%s' contains non-relative elements", ZSTR_VAL(time_str));
+		RETVAL_FALSE;


Also, we have the zend_argument_error function which serves as the quasi-standard way to handle argument related errors. PS. exact string values are usually not exposed in error messages.

kocsismate · 2023-01-20T21:15:36Z

ext/date/php_date.c

+	if (ce->type == ZEND_INTERNAL_CLASS) {
+		zend_throw_error(date_ce_date_object_error, "The %s object has not been correctly initialized by its constructor by calling parent::__construct()", ZSTR_VAL(ce->name));
+	} else {


Slightly different, maybe easier-to-read alternative for the message:

Object of type %s has not been initialized by calling parent::__construct() in its constructor.

(Object of type wording is used all over php-src)

kocsismate · 2023-01-20T21:19:37Z

ext/date/php_date.c

+		if (ce_ptr->type != ZEND_INTERNAL_CLASS) {
+			zend_throw_error(date_ce_date_object_error, "The %s object has not been correctly initialized by its constructor by calling parent::__construct()", ZSTR_VAL(ce->name));
+		}
+		zend_throw_error(date_ce_date_object_error, "The %s object (inheriting %s) has not been correctly initialized by its constructor by calling parent::__construct()", ZSTR_VAL(ce->name), ZSTR_VAL(ce_ptr->name));


nit: fwiw I'm not sure I'm curious about the parent class name, it's usually easy to find out from the name or at worst case from the signature of the child.

I find it helpful.

arnaud-lb · 2023-01-21T14:19:57Z

ext/date/php_date.c

+			ce_ptr = ce_ptr->parent;
+		}
+		if (ce_ptr->type != ZEND_INTERNAL_CLASS) {
+			zend_throw_error(date_ce_date_object_error, "The %s object has not been correctly initialized by its constructor by calling parent::__construct()", ZSTR_VAL(ce->name));


Is this reachable?

arnaud-lb · 2023-01-21T14:22:31Z

ext/date/php_date.c

@@ -307,9 +309,25 @@ static zend_object_handlers date_object_handlers_timezone;
 static zend_object_handlers date_object_handlers_interval;
 static zend_object_handlers date_object_handlers_period;

-#define DATE_CHECK_INITIALIZED(member, class_name) \
+static void date_throw_error_with_parent_name(zend_class_entry *ce)


Nit: A name like date_throw_uninitialized_error would be clearer IMHO

ext/date/php_date.c

arnaud-lb · 2023-01-21T15:00:46Z

ext/date/php_date.c

-		php_error_docref(NULL, E_WARNING, "Trying to compare different kinds of DateTimeZone objects");
+		zend_throw_error(date_ce_date_exception, "Cannot compare two different kinds of DateTimeZone objects");


I could not find this one in the RFC.

This can be triggered without using the OO API:

$o1 = timezone_open("GMT+1"); $o2 = timezone_open("Europe/Paris"); var_dump($o1 > $o2);

This is not the function API either, though.

This seems sensible/useful, as a failure in $o1 > $o2 is not observable unless an exception is thrown.

It's not in the RFC, but as you say, without doing this you can't observe this. I think I should leave this as is now.

ext/date/tests/bug52062.phpt

derickr · 2023-02-07T13:34:07Z

Unless there are more comments, I am going to merge this in the next day or so.

Girgias

Other than a small nit LGTM.

Girgias · 2023-02-07T14:07:55Z

ext/date/tests/DateInterval_createFromDateString_broken.phpt

+} catch (DateMalformedIntervalStringException $e) {
+	echo $e::class, ': ', $e->getMessage(), "\n";
+}
+var_dump($i);


Is this var_dump really necessary? Or move it within the try block to prevent the undefined variable warning

No, it slipped through. I'll remove it before merging.

TimWolla

I don't have any further remarks.

Girgias · 2023-02-08T01:47:19Z

ext/date/tests/DateInterval_createFromDateString_broken.phpt

@@ -7,10 +7,6 @@ try {
 } catch (DateMalformedIntervalStringException $e) {
 	echo $e::class, ': ', $e->getMessage(), "\n";
 }
-var_dump($i);
 ?>
 --EXPECTF--


Nit: this can now be EXPECT instead of EXPECTF :)

VincentLanglet · 2024-06-16T12:02:00Z

ext/date/php_date.stub.php

@@ -377,7 +377,6 @@ public function format(string $format): string {}

    /**
     * @tentative-return-type
-     * @alias date_modify
     */
    public function modify(string $modifier): DateTime|false {}


Hi @derickr, looking at https://3v4l.org/iSPL9,
it doesn't seem possible for

Datetime::modify DatetimeImmutable::modify DateInterval::createFromDateString

to return false anymore.

Shouldn't the stub be updated then ?

@VincentLanglet Good catch, I've just filed #14600 fixing these return types.

derickr added RFC Extension: date labels Jan 18, 2023

derickr self-assigned this Jan 18, 2023

github-actions bot added the Extension: intl label Jan 18, 2023

derickr commented Jan 20, 2023

View reviewed changes

ext/date/php_date.c Show resolved Hide resolved

ext/date/php_date.c Show resolved Hide resolved

ext/date/php_date.c Show resolved Hide resolved

ext/date/php_date.c Show resolved Hide resolved

ext/date/php_date.c Show resolved Hide resolved

derickr requested a review from TimWolla January 20, 2023 17:03

derickr changed the title ~~WIP: Implement DateTime Exceptions RFC~~ Implement DateTime Exceptions RFC Jan 20, 2023

derickr requested review from arnaud-lb, bukka and Girgias January 20, 2023 17:03

Girgias reviewed Jan 20, 2023

View reviewed changes

TimWolla reviewed Jan 20, 2023

View reviewed changes

kocsismate reviewed Jan 20, 2023

View reviewed changes

arnaud-lb reviewed Jan 27, 2023

View reviewed changes

TimWolla reviewed Jan 27, 2023

View reviewed changes

ext/date/tests/bug52062.phpt Outdated Show resolved Hide resolved

derickr mentioned this pull request Jan 30, 2023

Add PHP Core Round #9 ThePHPF/thephp.foundation#74

Merged

Girgias approved these changes Feb 7, 2023

View reviewed changes

TimWolla self-requested a review February 7, 2023 15:09

TimWolla reviewed Feb 7, 2023

View reviewed changes

derickr force-pushed the rfc-datetime-exceptions branch from f84bfe3 to 8bc4d47 Compare February 7, 2023 19:25

Girgias reviewed Feb 8, 2023

View reviewed changes

Implement More Appropriate Date/Time Exceptions RFC

b7860cd

derickr force-pushed the rfc-datetime-exceptions branch from 8bc4d47 to b7860cd Compare February 8, 2023 10:27

derickr merged commit 66a1a91 into php:master Feb 8, 2023

VincentLanglet reviewed Jun 16, 2024

View reviewed changes

VincentLanglet mentioned this pull request Jun 18, 2024

Update date exception thrown JetBrains/phpstorm-stubs#1646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement DateTime Exceptions RFC #10366

Implement DateTime Exceptions RFC #10366

derickr commented Jan 18, 2023 •

edited

Loading

Girgias Jan 20, 2023

kocsismate Jan 20, 2023

derickr Jan 26, 2023

Girgias Jan 20, 2023

derickr Jan 26, 2023

Girgias Jan 20, 2023

kocsismate Jan 20, 2023

kocsismate Jan 20, 2023

derickr Jan 26, 2023

Girgias Jan 20, 2023

derickr Jan 26, 2023

TimWolla left a comment

kocsismate Jan 20, 2023

derickr Jan 26, 2023

kocsismate Jan 20, 2023

kocsismate Jan 20, 2023

kocsismate Jan 20, 2023

kocsismate Jan 20, 2023

derickr Jan 26, 2023

arnaud-lb Jan 21, 2023

arnaud-lb Jan 21, 2023

derickr Jan 30, 2023

arnaud-lb Jan 21, 2023

derickr Jan 30, 2023

derickr commented Feb 7, 2023

Girgias left a comment

Girgias Feb 7, 2023

derickr Feb 7, 2023

TimWolla left a comment

Girgias Feb 8, 2023

VincentLanglet Jun 16, 2024

kocsismate Jun 18, 2024

		php_error_docref(NULL, E_WARNING, "Trying to compare different kinds of DateTimeZone objects");
		zend_throw_error(date_ce_date_exception, "Cannot compare two different kinds of DateTimeZone objects");

Implement DateTime Exceptions RFC #10366

Implement DateTime Exceptions RFC #10366

Conversation

derickr commented Jan 18, 2023 • 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

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

TimWolla left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derickr commented Feb 7, 2023

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimWolla left a 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 commented Jan 18, 2023 •

edited

Loading