Opened 11 years ago
Last modified 5 years ago
#26809 new defect (bug)
Errors from wp_update_post obscured in edit_post
Reported by: | 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:
- Activate a theme that has page templates (e.g. twentyeleven).
- Enable Jetpack and configure the Sharing module, which does things on
save_post
(or set up something else that does things onsave_post
). - Write a post and select a template (e.g.
single-page.php
). - Switch to a theme (e.g. twentythirteen) that doesn't have page template UX in the page editing screen.
- 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). - 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)
Change History (7)
#1
@
11 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
@
11 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.
#3
@
11 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
@
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 inwp-includes/post.php
that checks for a valid page template returns before several actions (includingsave_post
) fire because the valid template check fails.
Related: #29269
Changes that sort of fix the example (not submitted as a true fix, though)