Add some ValueErrors to ext/date by Girgias · Pull Request #5613 · php/php-src

Girgias · 2020-05-22T15:11:19Z

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?

derickr · 2020-05-24T13:11:25Z

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.

derickr · 2020-05-24T13:11:39Z

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.

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?

derickr · 2020-09-10T09:09:10Z

In any case, either do it, or don't add a comment.

derickr · 2020-09-10T09:11:21Z

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.

derickr

I made some minor new comments, but there are a few questions open from previous reviews wrt to idate.

…mp_get()

…d ticket

derickr

Except for my nit, and ignore the return type hint comments :-)

derickr · 2020-09-15T23:17:11Z

 ta.tm_zone = offset->abbr;
 #endif
 	}
-


nit: don't remove this empty line

derickr · 2020-09-15T23:18:50Z


 /**
- * @return int|false
+ * @return int


Now it returns just an "int", can it not be set as a return type on the function (line 236) itself?

derickr · 2020-09-15T23:18:58Z


 /**
- * @return int|false
+ * @return int


Girgias mentioned this pull request May 22, 2020
Change warnings to Errors in Date extension #4976
Closed

kocsismate reviewed May 23, 2020
View reviewed changes

Comment threadext/date/php_date.c Outdated
Comment threadext/date/php_date.c Outdated

Girgias force-pushed the date-argument-errors branch from 5b5fc69 to 71d0b18Compare May 24, 2020 12:09

derickr requested changes May 24, 2020
View reviewed changes

nikic added the Warning promotion label Aug 6, 2020

Girgias mentioned this pull request Aug 13, 2020
Warning to Error (TypeError/ValueError) in extensions php/php-tasks#18
Closed
71 tasks

Girgias force-pushed the date-argument-errors branch from 71d0b18 to fef9ba0Compare September 9, 2020 14:00

Girgias requested a review from derickr September 9, 2020 15:47

Girgias force-pushed the date-argument-errors branch from fef9ba0 to 8475547Compare September 9, 2020 22:34

derickr requested changes Sep 10, 2020
View reviewed changes

Girgias force-pushed the date-argument-errors branch 2 times, most recently from ee82bda to c1b5a49Compare September 13, 2020 18:57

derickr requested changes Sep 14, 2020
View reviewed changes

Comment threadext/date/php_date.c Outdated
Comment threadext/date/php_date.c Outdated
Comment threadext/date/php_date.c Outdated
Comment threadext/date/php_date.stub.php Outdated
Comment threadext/date/php_date_arginfo.h Outdated

Girgias force-pushed the date-argument-errors branch 7 times, most recently from a9a6628 to 9a54cd7Compare September 15, 2020 17:52

Girgias added 10 commits September 15, 2020 21:10

Add some ValueErrors to ext/date
68f47b6

Fix null byte comment
1e3509b

Fix stubs/arginfo
b6fea36

Make notice in timezone_identifiers_list() into a ValueError
6f8f866

Add warnings for when Epoch doesn't fit in a PHP integer
48240d5

Make epoch doesn't fit into PHP String a value error for date_timesta…
2e24885
…mp_get()

Drop todo comment because it's impossible to find the new link to sai…
aa48d8e
…d ticket

Drop comment and rename error1 var
4f9cae2

Revert idate() changes
9362d5a

Fix OpCache
a72f3cb

Fix 32bit tests
52593f5

Girgias force-pushed the date-argument-errors branch from 9a54cd7 to 52593f5Compare September 15, 2020 19:10

derickr approved these changes Sep 15, 2020
View reviewed changes

php-pulls closed this in 95f4ee3Sep 15, 2020

Girgias deleted the date-argument-errors branch September 15, 2020 23:30

Girgias mentioned this pull request Aug 19, 2024
Use a common function for "cannot be empty" ValueError #15489
Merged

Uh oh!

Conversation

Girgias commented May 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants