-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REST API: Meta update fails if unrelated unchanged field is a multi-item array #7149
base: trunk
Are you sure you want to change the base?
REST API: Meta update fails if unrelated unchanged field is a multi-item array #7149
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
src/wp-includes/meta.php
Outdated
* @return int|bool The new meta field ID if a field with the given key didn't exist | ||
* and was therefore added, true on successful update, | ||
* false on failure or if the value passed to the function | ||
* is the same as the one that is already in the database. | ||
*/ | ||
function update_metadata( $meta_type, $object_id, $meta_key, $meta_value, $prev_value = '' ) { | ||
function update_metadata( $meta_type, $object_id, $meta_key, $meta_value, $prev_value = '', &$is_failure = null ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be easier, to get this function to return a WP_Error object, using a parameter. Like how wp_insert_post
works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review, @spacedmonkey.
I've been considering this option.
update_metadata()
is internally used in the following functions:
update_comment_meta()
update_site_meta()
update_post_meta()
update_term_meta()
update_user_meta()
Therefore, if the return value of update_metadata()
changes, the return value of all these functions will also change.
According to this data, there are 2 352 WordPress extensions that use return value of one or more of these functions.
As a result, all these plugins would need to be tested, and some might need to be refactored.
Thus, I assume allowing update_metadata()
to return a WP_Error
object could break a lot of things.
So, I decided to add an additional function argument, as this is the least disruptive way to detect when update_metadata()
's database request fails (as opposed to a request that simply didn’t update any database rows).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anton-vlasenko To be clear, the parameter would be opt in, like this bool $wp_error = false
. So no need to change the parmaters of existing functions like this update_comment_meta
. As the rest api meta api, use the raw functions, like update_metadata
these functions can be opt-in. See https://core.trac.wordpress.org/ticket/15036
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, my mistake, @spacedmonkey. For some reason, I missed that the wp_insert_post()
function already has a $wp_error
parameter.
TL;DR: I agree with your suggestion and like the approach, especially since wp_insert_post()
already has a similar mechanism in place.
Fixed in f556bfd.
Longer explanation: I had considered this approach previously but dismissed it, thinking that in the distant future, update_metadata()
might need to return additional values (similar to how it should return a WP_Error object on database failure now).
This would potentially require two opt-in parameters, such as $wp_error
and maybe $wp_return_additional_data
or something similar.
With two opt-in parameters, it might become unclear which data to return if both are set to true. However, this is purely hypothetical and unlikely to occur.
… optional $rows_updated parameter.
…lps to determine if the actual DB error has occurred.
…ves. Add a comment.
…ame. Reason: failing unit tests.
…the update_metadata method.
1f01e06
to
638de3f
Compare
3c698b7
to
f556bfd
Compare
$result = add_metadata( $meta_type, $object_id, $raw_meta_key, $passed_value ); | ||
// The return value of add_metadata() should also be checked, as it may indicate a failure. | ||
if ( $wp_error && false === $result ) { | ||
return new WP_Error( 'meta_add_error', __( 'Failed to add metadata.' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what would be the best name for that error code.
@@ -306,6 +314,11 @@ function update_metadata( $meta_type, $object_id, $meta_key, $meta_value, $prev_ | |||
} | |||
|
|||
$result = $wpdb->update( $table, $data, $where ); | |||
|
|||
if ( $wp_error && false === $result ) { | |||
return new WP_Error( 'meta_update_error', __( 'Failed to update metadata.' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
Trac ticket: https://core.trac.wordpress.org/ticket/60618
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.