-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Implement DateTime Exceptions RFC #10366
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
Conversation
ext/date/php_date.c
Outdated
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ext/date/php_date.c
Outdated
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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(); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REVAL_FALSE;
should be unnecessary here as you are throwing an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RETURN_THROWS
should be used here and above as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fatal error: Uncaught DateException: Cannot compare two different kinds of DateTimeZone objects in %s | ||
Stack trace: | ||
#0 {main} | ||
thrown in %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap this call in a try/catch and echo $e::class, ': ', $e->getMessage(), PHP_EOL;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - it also missed the last assertion in this test due to the fatal error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_create_from_date_string_nullparam.phpt
Outdated
Show resolved
Hide resolved
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now you don't want to specify the exact class name? (the same above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__set_state
and __wakeup
are static methods, so how can it be any other class?
ext/date/php_date.c
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RETURN_THROWS
should be used here and above as well
ext/date/php_date.c
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ext/date/php_date.c
Outdated
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
ext/date/php_date.c
Outdated
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it helpful.
ext/date/php_date.c
Outdated
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this reachable?
ext/date/php_date.c
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: A name like date_throw_uninitialized_error
would be clearer IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Unless there are more comments, I am going to merge this in the next day or so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than a small nit LGTM.
} catch (DateMalformedIntervalStringException $e) { | ||
echo $e::class, ': ', $e->getMessage(), "\n"; | ||
} | ||
var_dump($i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this var_dump really necessary? Or move it within the try block to prevent the undefined variable warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it slipped through. I'll remove it before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any further remarks.
f84bfe3
to
8bc4d47
Compare
@@ -7,10 +7,6 @@ try { | |||
} catch (DateMalformedIntervalStringException $e) { | |||
echo $e::class, ': ', $e->getMessage(), "\n"; | |||
} | |||
var_dump($i); | |||
?> | |||
--EXPECTF-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this can now be EXPECT instead of EXPECTF :)
8bc4d47
to
b7860cd
Compare
@@ -377,7 +377,6 @@ public function format(string $format): string {} | |||
|
|||
/** | |||
* @tentative-return-type | |||
* @alias date_modify | |||
*/ | |||
public function modify(string $modifier): DateTime|false {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VincentLanglet Good catch, I've just filed #14600 fixing these return types.
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 :-)