Add some ValueErrors to ext/date #5613
Conversation
Girgias
force-pushed
the
date-argument-errors
branch
from
5b5fc69 to
71d0b18
Compare
ext/date/tests/005.phpt
Outdated
| var_dump(idate(0)); | ||
| try { | ||
| var_dump(idate(1,1)); | ||
| } catch (\ValueError $e) { |
There was a problem hiding this comment.
I am not keen on this change, as this is a BC break. Previously wrong values just returned false, which is something that strtotime and the older date functions have done for decades. Upgrading them to throw an exception is not a great thing to do here. It's going to cause BC breaks, even if people already carefully checked the return values. As most input to date functions will come from either user input or a database, I don't believe this is a worthy place to introduce breaks.
Instead, we should point people to the DateTime classes.
ext/date/tests/009.phpt
Outdated
| var_dump(strftime("")); | ||
| } catch (\ValueError $e) { | ||
| echo $e->getMessage() . \PHP_EOL; | ||
| } |
There was a problem hiding this comment.
I am not keen on this change, as this is a BC break. Previously wrong values just returned false, which is something that strtotime and the older date functions have done for decades. Upgrading them to throw an exception is not a great thing to do here. It's going to cause BC breaks, even if people already carefully checked the return values. As most input to date functions will come from either user input or a database, I don't believe this is a worthy place to introduce breaks.
Instead, we should point people to the DateTime classes.
There was a problem hiding this comment.
Are you trying to promote the use of the DateTim(Immutable) class for this specific case, or more generally?
From what I've understood the DateTime classes also return false and don't throw, or would you be more inclined for the objects to throw and be able to drop the false return?
Girgias
force-pushed
the
date-argument-errors
branch
from
71d0b18 to
fef9ba0
Compare
Girgias
force-pushed
the
date-argument-errors
branch
from
fef9ba0 to
8475547
Compare
ext/date/php_date.c
Outdated
| @@ -4408,6 +4425,7 @@ PHP_FUNCTION(timezone_identifiers_list) | |||
|
|
|||
| /* Extra validation */ | |||
| if (what == PHP_DATE_TIMEZONE_PER_COUNTRY && option_len != 2) { | |||
| // Promoto to ValueError? | |||
There was a problem hiding this comment.
In any case, either do it, or don't add a comment.
ext/date/php_date.c
Outdated
| if (error) { | ||
| /* TODO Add warning? */ |
There was a problem hiding this comment.
I'm thinking a ValueException or something is OK here, as the function/method specifically asks the code to return an integer since the epoch.
Girgias
force-pushed
the
date-argument-errors
branch
2 times, most recently
from
ee82bda to
c1b5a49
Compare
Girgias
force-pushed
the
date-argument-errors
branch
7 times, most recently
from
a9a6628 to
9a54cd7
Compare
Girgias
force-pushed
the
date-argument-errors
branch
from
9a54cd7 to
52593f5
Compare
| @@ -1224,7 +1239,6 @@ PHPAPI void php_strftime(INTERNAL_FUNCTION_PARAMETERS, int gmt) | |||
| ta.tm_zone = offset->abbr; | |||
| #endif | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: don't remove this empty line
| @@ -230,7 +230,7 @@ public function setISODate(int $year, int $week, int $dayOfWeek = 1) {} | |||
| public function setTimestamp(int $timestamp) {} | |||
|
|
|||
| /** | |||
| * @return int|false | |||
| * @return int | |||
There was a problem hiding this comment.
Now it returns just an "int", can it not be set as a return type on the function (line 236) itself?
| @@ -282,7 +282,7 @@ public function getTimezone() {} | |||
| public function getOffset() {} | |||
|
|
|||
| /** | |||
| * @return int|false | |||
| * @return int | |||
A new spin on #4976 which is more conservative.
This allows to drop a couple of false returns.
@derickr is this change OK with you?