Make WordPress Core

Opened 10 years ago

Closed 5 years ago

Last modified 2 years ago

#28992 closed defect (bug) (fixed)

mysql2date() return empty string not false sometimes added doing it wrong message

Reported by: pbearne's profile pbearne Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.8
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

If you pass a empty date string or bad date to mysql2date() we get a empty string back

e.g.

mysql2date( 'Y-m-d', "" )

mysql2date( 'Y-m-d', 'i am not a date' )

see the unit tests for more examples

I have created patch to return false if it not going to return a date as this is safer i belive

Attachments (5)

mysql2date.patch (4.0 KB) - added by pbearne 10 years ago.
mysql2date.patch with UnitTest and function patch
mysql2date.2.patch (3.0 KB) - added by pbearne 10 years ago.
removed commented code and added doc block with ticket no
28992.patch (9.5 KB) - added by pbearne 8 years ago.
complete patch for mysql2date() and Unit tests adding _doing_it_wrong call to warning about bad output
mysql2date-revamp.patch (4.7 KB) - added by Rarst 5 years ago.
xmlrpc-default-new-post-curernt-time.patch (1.1 KB) - added by Rarst 5 years ago.

Download all attachments as: .zip

Change History (29)

@pbearne
10 years ago

mysql2date.patch with UnitTest and function patch

#1 @pbearne
10 years ago

  • Keywords has-patch needs-testing added

@pbearne
10 years ago

removed commented code and added doc block with ticket no

This ticket was mentioned in IRC in #wordpress-dev by pbearne. View the logs.


10 years ago

This ticket was mentioned in IRC in #wordpress-dev by pbearne. View the logs.


10 years ago

This ticket was mentioned in IRC in #wordpress-dev by pbearne. View the logs.


10 years ago

This ticket was mentioned in IRC in #wordpress-dev by pbearne. View the logs.


10 years ago

#6 @tellyworth
10 years ago

  • Version changed from trunk to 3.8

#7 @welcher
8 years ago

  • Keywords dev-feedback added

#8 @aaroncampbell
8 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

The inline documentation for mysql2date was updated in [29344] as part of #28310, and two unit tests were added.

My initial thought was that I didn't want to change the return in any case, but instead just document them. Having said that, I don't like how inconsistent this is:

mysql2date( 'Y-m-d', '' ); // false - expected

mysql2date( 'U', 'not a date' ); // false - expected
mysql2date( 'G', 'not a date' ); // false - expected
mysql2date( 'Y-m-d', 'not a date' ); // "2015-10-04" (current date)?!

mysql2date( 'U', '2012-12-30' ); // 1356825600 - expected
mysql2date( 'G', '2012-12-30' ); // 1356825600 - expected
mysql2date( 'Y-m-d', '2012-12-30' ); // "2012-12-30" - expected

mysql2date( 'U', '1' ); // false - expected
mysql2date( 'G', '1' ); // false - expected
mysql2date( 'Y-m-d', '1' ); // "2015-10-04" (current date)?!

I don't like how "1" and "not a date" return false if the format string is U or G, and current date for any other format string. Does anyone have any particularly compelling reasons for having dates that strtotime doesn't understand return false vs empty string or anything else?

#9 @aaroncampbell
8 years ago

Sorry, I left out an important bit:
An invalid date, with a format string other than U or G has returned the current date for a very long time with this function (pre WordPress 1.0).

#10 @pbearne
8 years ago

There were 13 failures when incorrect usage notice added

1) Tests_WP_Date_Query::test_validate_date_values_should_process_array_value_for_year
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

2) Tests_WP_Date_Query::test_validate_date_values_should_process_array_value_for_day
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

3) Tests_WP_Date_Query::test_validate_date_values_should_process_array_value_for_day_when_values_are_invalid
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

4) Tests_Query_Conditionals::test_search_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

5) Tests_Query_Conditionals::test_category_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

6) Tests_Query_Conditionals::test_tag_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

7) Tests_Query_Conditionals::test_author_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

8) Tests_Query_VerbosePageRules::test_search_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

9) Tests_Query_VerbosePageRules::test_category_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

10) Tests_Query_VerbosePageRules::test_tag_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

11) Tests_Query_VerbosePageRules::test_author_feed
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

12) Tests_XMLRPC_wp_newPost::test_invalid_post_date_does_not_fatal
Unexpected incorrect usage notice for mysql2date

/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:278
/srv/www/wordpress-develop/tests/phpunit/includes/testcase.php:84

13) Tests_XMLRPC_wp_newPost::test_invalid_post_date_gmt_does_not_fatal
Unexpected incorrect usage notice for mysql2date
Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#11 @pbearne
8 years ago

Added a _doing_it_wrong() call to mysql2date so we can at a later time change it to a return false if a bad date is passed instead of the current state where an empty string can returned

Added $this->setExpectedIncorrectUsage( 'mysql2date' ); to all the test cases that where failing after I added the do it wrong

Moved two already existing tests for mysql2date() from post to the new test class

This function should fixed in a later version

@pbearne
8 years ago

complete patch for mysql2date() and Unit tests adding _doing_it_wrong call to warning about bad output

#12 @pbearne
8 years ago

  • Keywords has-patch has-unit-tests added; needs-refresh removed

#13 @pbearne
8 years ago

  • Summary changed from mysql2date() return empty string not false to mysql2date() return empty string not false sometimes added doing it wrong message

#14 @Rarst
5 years ago

  • Keywords needs-testing dev-feedback removed

Ok, added a patch with fresh look in unit tests and revamp for the function.

My thinking is this function is only ever meant to eat MySQL formatted times. As with many other date/time functions it will accept other stuff, but in this case you are definitely on your own if you are doing strange things with it.

For same reason I dropped doing it wrong, since I don't think this warrants special handling and messaging.

I focused unit tests accordingly and made false return for any input that date_create() couldn't deal with.

I also re-documented it, since documentation was really just making confusing claims about what it does. While G and U were supposed to be somehow different timestamp return, in practice code produced exactly same return for them.

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


5 years ago

#16 @SergeyBiryukov
5 years ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#17 @pbearne
5 years ago

the Patch looks good just happy to see it moving

#18 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 45908:

Date/Time: Revamp mysql2date() to use wp_date() and handle invalid input in a consistent manner.

Add unit tests, improve documentation.

Props Rarst, pbearne.
Fixes #28992.

#19 follow-up: @SergeyBiryukov
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

2 failures in XML-RPC tests:

1) Tests_XMLRPC_wp_newPost::test_invalid_post_date_does_not_fatal
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1970-01-01 00:00:00'
+'2019-08-29 05:13:46'
/var/www/tests/phpunit/tests/xmlrpc/wp/newPost.php:358
2) Tests_XMLRPC_wp_newPost::test_invalid_post_date_gmt_does_not_fatal
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1970-01-01 00:00:00'
+'0000-00-00 00:00:00'
/var/www/tests/phpunit/tests/xmlrpc/wp/newPost.php:375

The first result is for invalid_date (with underscore), the second is for invalid date (with a space).

Are these expected?

#20 @Rarst
5 years ago

Ugh. I did change behavior on invalid input, the problem is function behaves differently on different invalid input.

I'll look at the test and probably restore the behavior (keeping the new implementation though, because it does solve other issues). I've been thinking I need to restore old behavior in localization as well.

Last edited 5 years ago by Rarst (previous) (diff)

#21 @Rarst
5 years ago

Ok, so the old behavior is compound failure really:

  1. strtotime() fails and returns false.
  2. date() receives false instead of timestamp, but it gets cast to 0

This causes XML RPC to create post with invalid date as 1970-01-01

However this isn't normal for creating WP posts, they would default to current date without date input.

So making mysql2date fail consistently makes XMLRPC behavior consistent with "normal" post creation.

Since the failing tests were originally meant to test for error, not specifically to codify zero time as correct outcome, I suggest we update the tests.

#22 in reply to: ↑ 19 @SergeyBiryukov
5 years ago

Replying to SergeyBiryukov:

The first result is for invalid_date (with underscore), the second is for invalid date (with a space).

Ah, looks like it's not that. It's because the first test is for post_date and the second one is for post_date_gmt, which is handled differently if an invalid date is passed.

The resulting post data before [45908]:

post_date: 1970-01-01 00:00:00
post_date_gmt: 1970-01-01 00:00:00

After [45908]:

post_date: 2019-08-29 05:13:46
post_date_gmt: 0000-00-00 00:00:00

There's some logic in convert_date_gmt() to deal with the latter case.

Still, as the tests were originally meant to check for the lack of a fatal error, not specifically mark a particular outcome as the correct one, I agree with updating the tests.

#23 @SergeyBiryukov
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 45911:

Date/Time: Update XML-RPC tests for invalid date input to account for a more consistent mysql2date() error handling in [45908].

The tests were meant to check for the lack of a fatal error in case of invalid date input, not specifically mark a particular outcome as the correct one.

Props Rarst.
Fixes #28992.

#24 @johnbillion
2 years ago

In 51950:

Date/Time: Improve the docblocks for various date and time related functions.

See #53399, #28992, #40653

Note: See TracTickets for help on using tickets.