Make WordPress Core

Opened 10 years ago

Last modified 5 years ago

#26809 new defect (bug)

Errors from wp_update_post obscured in edit_post

Reported by: dllh's profile dllh Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.9
Component: Posts, Post Types Keywords: has-patch
Focuses: Cc:

Description

A sample reproduction of a bug that illustrates the general case I'd like to document:

  1. Activate a theme that has page templates (e.g. twentyeleven).
  2. Enable Jetpack and configure the Sharing module, which does things on save_post (or set up something else that does things on save_post).
  3. Write a post and select a template (e.g. single-page.php).
  4. Switch to a theme (e.g. twentythirteen) that doesn't have page template UX in the page editing screen.
  5. Toggle the "Show sharing buttons" setting in the relevant metabox (or do something else that will occur on save_post that's easily detected after saving).
  6. Save the post.

Expected Result: If there's a failure saving any piece of the data, the user'll be notified, and possibly the whole save will be postponed until any errors are corrected.

Actual Result: Changes to the main post fields are saved, but things that would have fired on save_post do not happen, resulting in a partial save of the data (e.g. sharing status is not saved). The code in wp-includes/post.php that checks for a valid page template returns before several actions (including save_post) fire because the valid template check fails. The failure is essentially silent because edit_post() doesn't check the return of wp_update_post().

In the particular case I outline above, the issue arises because post meta for the page template existed and lives in $postarr based on a save when the old theme was activated. When saved under a theme with no page template UX, the value from meta is used but is not a valid value for the new theme. So it correctly errors, but the early return causes later actions not to fire properly, wreaking havoc that's tricky to spot.

Optimally, it seems like WP would at least check the return of wp_update_post( $foo, true ) for is_wp_error() in edit_post() and would fail gracefully (or at least wp_die() as in other cases) if an error was returned, but then there are other issues as well, such as the fact that some meta values are saved in edit_post() before the wp_update_post() executes, and theoretically these'd need to be rolled back as well on failure.

The template check in wp-includes/post.php should also be moved to higher up in the code than where it currently lives so that the check would bail prior to the database update.

I imagine that there might be much larger architectural issues that would subsume any changes to the way edit_post() works, so rather than providing a fix for the specific issue that'd likely be rejected anyway, I wanted to raise the general issue for discussion/rejection/whatever. Possibly relevant: #21963.

Attachments (2)

26809.patch (1.8 KB) - added by dllh 10 years ago.
Changes that sort of fix the example (not submitted as a true fix, though)
26809.1.patch (1.1 KB) - added by bobbingwide 10 years ago.
Do not fail when theme does not support page templates

Download all attachments as: .zip

Change History (7)

@dllh
10 years ago

Changes that sort of fix the example (not submitted as a true fix, though)

#1 @nacin
10 years ago

  • Component changed from General to Post Types
  • Milestone changed from Awaiting Review to Future Release

I definitely like the page template change. This is general to wp_imsert_post() and I believe duplicates another ticket (but ai like this patch in particular).

As for edit_post(), given all the fun things that can be returned as an error, I don't love suddenly turning those into wp_die() situations. Open to suggestions.

#2 @bobbingwide
10 years ago

For me the expected result is that there should not be an error due to the new theme not supporting the _wp_page_template post meta data.

I would simply wrap the code after get_page_templates() to only continue if there are any defined for the current theme.

    $page_templates = wp_get_theme()->get_page_templates();
    if ( count( $page_templates ) ) {
		  if ( 'default' != $page_template && ! isset( $page_templates[ $page_template ] ) ) {
		  	if ( $wp_error )
		  		return new WP_Error('invalid_page_template', __('The page template is invalid.'));
		  	else
		  		return 0;
		  } 
		  update_post_meta($post_ID, '_wp_page_template',  $page_template);
    } 

If the user switches back to the original theme then they can continue where they left off.
If they switch to another theme that supports page templates and the chosen template does not exist in this theme then they might see errors.

@bobbingwide
10 years ago

Do not fail when theme does not support page templates

#3 @bobbingwide
10 years ago

  • Severity changed from normal to major

In my experience, this bug can lead to loss of data.
It prevents post meta data from being saved, with no message to the user.
By not creating an error my patch allows the updates that the user made to be applied.
So data is not lost.

In the announcement for WordPress 3.8.3 it was stated that "any loss of content is unacceptable". While I don't believe this bug deserves its own quickfix, I'd like it to be reconsidered.

#4 @SergeyBiryukov
10 years ago

Changes to the main post fields are saved, but things that would have fired on save_post do not happen, resulting in a partial save of the data (e.g. sharing status is not saved). The code in wp-includes/post.php that checks for a valid page template returns before several actions (including save_post) fire because the valid template check fails.

Related: #29269

#5 @chriscct7
8 years ago

  • Keywords has-patch added
  • Severity changed from major to normal
Note: See TracTickets for help on using tickets.