WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 weeks ago

#28992 closed defect (bug) (fixed)

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

Reported by: pbearne Owned by: 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 5 years ago.
mysql2date.patch with UnitTest and function patch
mysql2date.2.patch (3.0 KB) - added by pbearne 5 years ago.
removed commented code and added doc block with ticket no
28992.patch (9.5 KB) - added by pbearne 4 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 3 weeks ago.
xmlrpc-default-new-post-curernt-time.patch (1.1 KB) - added by Rarst 3 weeks ago.

Download all attachments as: .zip

Change History (28)

@pbearne
5 years ago

mysql2date.patch with UnitTest and function patch

#1 @pbearne
5 years ago

  • Keywords has-patch needs-testing added

@pbearne
5 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.


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#6 @tellyworth
5 years ago

  • Version changed from trunk to 3.8

#7 @welcher
4 years ago

  • Keywords dev-feedback added

#8 @aaroncampbell
4 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
4 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
4 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 4 years ago by SergeyBiryukov (previous) (diff)

#11 @pbearne
4 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
4 years ago

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

#12 @pbearne
4 years ago

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

#13 @pbearne
4 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
3 weeks 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.


3 weeks ago

#16 @SergeyBiryukov
3 weeks ago

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

#17 @pbearne
3 weeks ago

the Patch looks good just happy to see it moving

#18 @SergeyBiryukov
3 weeks 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
3 weeks 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
3 weeks 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 3 weeks ago by Rarst (previous) (diff)

#21 @Rarst
3 weeks 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
3 weeks 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
3 weeks 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.

Note: See TracTickets for help on using tickets.