Repository navigation
Conversation
There was a problem hiding this comment.
I think I would prefer it if timelib_time does not get (more) exposed to the external API. Would it perhaps make more sense to have this function accept a php_date_obj instead of a timelib_time?
There was a problem hiding this comment.
Agreed. I vaguely recall having talked about exactly that in room 11 actually.
I changed the signature and function name.
| if (instanceof_function_slow(Z_OBJCE_P(data), php_date_get_interface_ce())) { | ||
| php_date_obj *dateobj = Z_PHPDATE_P(data); | ||
| if (dateobj->time) { | ||
| zend_string *formatted = php_format_date_obj(ext_date_format, ext_date_format_len, dateobj); |
There was a problem hiding this comment.
You're testing for time here, but IMO that should be done in the new php_format_date_obj function, which, if it returns NULL, can then else into your error message branch?
There was a problem hiding this comment.
Right of course, that makes sense
Girgias
left a comment
There was a problem hiding this comment.
Ideally this should add a zend_module_dependency struct into the module entry, a bit like XMLReader:
static const zend_module_dep soap_deps[] = {
ZEND_MOD_REQUIRED("date")
ZEND_MOD_END
};|
I guess so? Although date is always available. And if it were not then the module won't even load: it would fail at dlopen even before the dependencies are checked so it is a bit pointless anyway to add the dependency requirement. |
|
FWIW, I don't think it would fail at |
Ah right, I somehow forgot this was the default instead of RTLD_NOW. Thanks for reviewing. |
Fixes https://bugs.php.net/bug.php?id=44383
Marking as draft as I've asked Derick for input on the date API.