Invalid published date insertion in posts

[hew]

To reproduce:

1. Set date on a post to Feb 29th in a non-leap year.
   
2. Click "Update".
   

The UI JS will prevent you from hitting OK, but you can bypass it and insert the bad datestamp as described above. I get other inconsistant oddness using the same procedure with other bad numerals (54, -2, etc).

This can also be done via Quick Edits using the same procedure.

I didn't explore pages as thoroughly, but that's probably worth investigation too.

[solarissmoke]

Even if you bypass the JS, the date fields are validated server side and invalid values are ignored (so Feb 29, 2011 will not be saved as a future publish date). So I don't see what the problem is?

[westi]

Replying to solarissmoke:

Even if you bypass the JS, the date fields are validated server side and invalid values are ignored (so Feb 29, 2011 will not be saved as a future publish date). So I don't see what the problem is?

We end up with a valid date in post_date_gmt and an invalid one in post_date.

I think with newer mysql the insertion may fail

[westi]

Example:

• Set offset to UTC+6
• Use mysql 5.1
• Create a post for 30th Feb 2011 - schedule and don't click ok / ignore the red warning.
• End up with permalink - ​http://example.com/-0001/11/30/another-post-from-a-dodgy-day/

[solarissmoke]

Ah, problem indeed. Here's a tentative patch - it disables the submit button until you click OK on the timestamp editor and the date is valid (this may need UI feedback), and issues an error server-side if an invalid date is supplied (for when JS is disabled).

[solarissmoke]

There was some unnecesasry JS in the previous patch

[hew]

Don't forget the edit.php using Quick Edit avenue.

[solarissmoke]

That one is tricky... edit_post() calls wp_die() when there is an error. In AJAX mode, wp_die() kills the error message and returns -1 (which list table JS doesn't handle too well right now, it just prints it). I don't think the behaviour of edit_post() can be changed because it is used in many places. We could:

• Just print a generic error response when the response is -1 (not very helpful to the user IMO)
• Add JS validation to the inline-edit form
• Change the inline-save procedure to call _wp_translate_post_data() manually and return any errors before calling edit_post()

I don't know which (if any) is a good idea.

[ryan]

We'll need to play nice with plugins that implement non-Gregorian calendars.

​http://wordpress.org/extend/plugins/wp-jalali/

[ryan]

No movement, possible plugin incompatibilities, and date/time always rouses dragons, so punting.

[jkudish]

In ​17180-3.diff:

• Check the dates via js whenever the post publishing form #post is submitted. If the dates are invalid, prevent publish, hide the ajax spinner, remove the disabled state from the publish button and slide down the #timestampdiv revealing the date error (highlited in red), giving the user a visual indication of the problem and an opportunity to fix it.
• Check the dates with php's native chekdate() function before saving a post and return a WP_Error if the date is wrong

Tested the patch with posts, pages and quick edit and it all works as expected.

In regards to:

Replying to ryan:

We'll need to play nice with plugins that implement non-Gregorian calendars.
​http://wordpress.org/extend/plugins/wp-jalali/

I tested with this plugin active, but it was hard to tell whether things we're behaving properly or not. Partially because I have no knowledge of how this calendar works or of arabic (which the plugin's text is in) and also because the plugin spits out a bunch of php notices and warnings. Nonetheless, I seemed to be able to save posts properly with the plugin active.

[westi]

Replying to jkudish:

In ​17180-3.diff:

• Check the dates via js whenever the post publishing form #post is submitted. If the dates are invalid, prevent publish, hide the ajax spinner, remove the disabled state from the publish button and slide down the #timestampdiv revealing the date error (highlited in red), giving the user a visual indication of the problem and an opportunity to fix it.
• Check the dates with php's native chekdate() function before saving a post and return a WP_Error if the date is wrong

Tested the patch with posts, pages and quick edit and it all works as expected.

This looks like a good first pass but I think it doesn't cover all possible post editing/insertion routes and so we could still have invalid dates coming in from other places because this is only really in the code flow for the admin ui.

We should protect against invalid dates from XML-RPC as well by adding a similar check lower down in the API inside wp_insert_post as this is the function that finally does the db writes.

In regards to:

Replying to ryan:

We'll need to play nice with plugins that implement non-Gregorian calendars.
​http://wordpress.org/extend/plugins/wp-jalali/

I tested with this plugin active, but it was hard to tell whether things we're behaving properly or not. Partially because I have no knowledge of how this calendar works or of arabic (which the plugin's text is in) and also because the plugin spits out a bunch of php notices and warnings. Nonetheless, I seemed to be able to save posts properly with the plugin active.

I think that as checkdate is specifically a Gregorian function we should either have a wrapper function with a filter in it or we should filter the response from it so that plugins that want to implement different calendar systems can check the date in there own way.

[jkudish]

Replying to westi:

We should protect against invalid dates from XML-RPC as well by adding a similar check lower down in the API inside wp_insert_post as this is the function that finally does the db writes.
I think that as checkdate is specifically a Gregorian function we should either have a wrapper function with a filter in it or we should filter the response from it so that plugins that want to implement different calendar systems can check the date in there own way.

Both these good points have been addressed in ​17180-4.diff

[jkudish]

Unit test that tests the proposed patch (​17180-4.diff) and confirms that a WP_Error is returned for invalid dates

[westi]

Unit Tests added in ​[UT1026]

[westi]

I gave this a quick test in trunk.

Apply using curl "http://core.trac.wordpress.org/raw-attachment/ticket/17180/17180-4.diff" | sed "s/.dev//" | patch -p0 to get around the script renaming.

The User Experience when I try and Save a Draft or Publish a post with a bad date it a little in-elegant.

I just end up on a wp_die page and loose my hard work, feels like we should be able to do something better than that as this is probably worse that the current experience of a whacky date in the db.

[jkudish]

Replying to westi:

The User Experience when I try and Save a Draft or Publish a post with a bad date it a little in-elegant.

I just end up on a wp_die page and loose my hard work, feels like we should be able to do something better than that as this is probably worse that the current experience of a whacky date in the db.

I think the patch might need an update because this is not the intended behaviour. Unless you had js turned off?

I'll check it again soon.

[westi]

Replying to westi:

I gave this a quick test in trunk.

Apply using curl "http://core.trac.wordpress.org/raw-attachment/ticket/17180/17180-4.diff" | sed "s/.dev//" | patch -p0 to get around the script renaming.

The User Experience when I try and Save a Draft or Publish a post with a bad date it a little in-elegant.

I just end up on a wp_die page and loose my hard work, feels like we should be able to do something better than that as this is probably worse that the current experience of a whacky date in the db.

Ignore me - I fail at late night testing. It works fine.

[westi]

In [21921]:

Posting: Make it much harder to create posts with invalid dates by enforcing the post date tests in the UI and the backend code.

Previously you could quite easily send a new post into the back of beyond by specifying an invalid date like the 30th Feb and this was very confusing.
Sometimes it would seem to work and sometimes the post would end up very far in the past - depending on the mysql version and other factors.

Fixes #17180 props jkudish.

[nacin]

These filters are two different names. They should probably be the same. I usually like filters over wrapper functions, but wp_checkdate() probably isn't a bad idea, here. I don't think the context (whether you came from wp_insert_post() or edit_post(), or what the post array is) is necessary, and if anything it is spurious. If you want to modify a date based on some context, there are other filters for that. I usually don't fight for less context, but the only context that should matter here is what calendar system is being used.

Also, wp_insert_post() can return 0 depending on $wp_error, so we need to avoid returning WP_Error in that case. And whoops has an "h". :-)

[jkudish]

Replying to nacin:

Also, wp_insert_post() can return 0 depending on $wp_error, so we need to avoid returning WP_Error in that case.

What would be a better return? false? 0?

[westi]

Improvements based on nacin's feedback

[westi]

Replying to nacin:

These filters are two different names. They should probably be the same. I usually like filters over wrapper functions, but wp_checkdate() probably isn't a bad idea, here. I don't think the context (whether you came from wp_insert_post() or edit_post(), or what the post array is) is necessary, and if anything it is spurious. If you want to modify a date based on some context, there are other filters for that. I usually don't fight for less context, but the only context that should matter here is what calendar system is being used.

Also, wp_insert_post() can return 0 depending on $wp_error, so we need to avoid returning WP_Error in that case. And whoops has an "h". :-)

Thanks :)

I think ​http://core.trac.wordpress.org/attachment/ticket/17180/17180.improvements.diff should address all your points - all further feedback?

[jkudish]

Replying to westi:

I think ​http://core.trac.wordpress.org/attachment/ticket/17180/17180.improvements.diff should address all your points - all further feedback?

Tested, looks good to me.

[westi]

In [21922]:

Posting: Improve the invalid date protection code based on feedback from nacin.

• Introduce a wp_checkdate() function with a single filter to centralise the code that validates dates.
• Improve the error message
• Correctly handle the return value of wp_insert_post which is not always a WP_Error on failure

Fixes #17180

[nacin]

In [21947]:

Revert JS from [21921] as it breaks saving when no post date fields are present. see #17180.

[jkudish]

Adjust the js originally introduced [21921] and reverted in [21947] to detect wether or not the date fields are present on the page or not, and only provide validation when the fields are present

[jkudish]

as suggested by nacin, make updateText() bail when there is no date/time field in the current page

[westi]

@jkudish: I tested the latest patch and it looks like it works but has confusing behaviour in current trunk because it kicks in after the spinner has been enabled and so you have a spinner still spinning.

[kovshenin]

@jkudish: unless I'm missing something, this might be a good time to change that to $.on instead of $.submit: ​http://api.jquery.com/on/

[wonderboymusic]

Tagging refresh to get an update on where this is

[jkudish]

refreshed patch which properly hides the spinner when a date error occurs

[jkudish]

just 17180.7.diff but now using jQuery's 'on' method instead of 'submit' as suggested by 'kovshenin'

[jkudish]

Newest patch should be good to go

[nacin]

westi: punt candidate in my eyes.

[jkudish]

westi: punt candidate in my eyes.

Curious why? The php part of this patch has been in for a while and seemingly works fine. The js part was working fine but broke due to some other js changes in core. It's now back to working fine and should be ok to commit. I think that if we are to punt this then we should revert the php changes. Because the experience of attempting a bad date and ending up on a wp_die screen (something the js otherwise prevents) is a poor experience. But of course, I would vote for not punting :)

[westi]

In 22442:

Post UI: Provide visual feedback to the user if they try to set an invalid date for a post. Fixes #17180 props jkudish.