Opened 7 years ago
Closed 6 years ago
#45206 closed defect (bug) (fixed)
Logic error in do_meta_boxes() for Gutenberg support
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.0 | Priority: | normal |
Severity: | normal | Version: | 5.0 |
Component: | Options, Meta APIs | Keywords: | has-patch fixed-5.0 |
Focuses: | Cc: |
Description (last modified by )
When testing 5.0 beta 1 on an existing error-free installation, I started seeing fatal errors in the logs of a multisite network I maintain.
To quickly duplicate, install and activate the WP User Profiles plugin (which uses meta-boxes for user profile pages) and visit your profile inside of WordPress admin.
PHP message: PHP Fatal error: Uncaught Error: Cannot use object of type WP_User as array in /public/wordpress/wp-admin/includes/template.php:1066
Specifically:
thrown in /public/wordpress/wp-admin/includes/template.php on line 1066" while reading upstream, client: 184.58.156.205, server: , request: "GET /wp-admin/users.php?page=profile HTTP/1.0", upstream: "fastcgi://unix:/var/run/php/php7.2-fpm.sock:", host: "jjj.blog", referrer: "https://jjj.blog/wp-admin/users.php?page=account"
In short, it is incorrect to assume that meta-boxes are always for posts, and that the args
key is always present, and is an array. (The is_array( $box['args'] )
check from 2016, r37972, supports this notion.)
Related to #45112, and r43779.
Patch with a recommended fix imminent.
Attachments (1)
Change History (17)
#4
@
7 years ago
In 45206.patch there seems to be a typo:
$block_compatible |= (bool) $box['args']['__back_compat_meta_box'];
This will not be triggered by the plugin though.
I assume it was meant to be:
$block_compatible != (bool) $box['args']['__back_compat_meta_box'];
(The
is_array( $box['args'] )
check from 2016, r37913, supports this notion.)
This changeset reference seems to be ajax related and unrelated to the ticket?
Also this changeset reference seems to be cron related and seems unrelated?
#5
@
7 years ago
@birgire
- Not a typo in the patch. That's the original code and is a bitwise comparison in PHP.
- I forget that Trac auto links to core and not develop. Sorry about that. Will fix.
#9
@
7 years ago
@johnjamesjacoby thanks updating the changeset references and thanks for the clarification, I totally missed that this comparison came from original code :-)
#10
follow-up:
↓ 11
@
7 years ago
@birgire let’s assume the bitwise operator is correct, but it might be a good idea to document it in the code as such. They aren’t popular, and I have a hunch you won’t be the last person to question it.
#11
in reply to:
↑ 10
@
6 years ago
Replying to johnjamesjacoby:
@birgire let’s assume the bitwise operator is correct, but it might be a good idea to document it in the code as such. They aren’t popular, and I have a hunch you won’t be the last person to question it.
Sounds like a good idea. Even though I've occasionally worked with bitwise operators, it's application in this context here didn't turn on the light bulb :-)
I also wonder if the add_meta_box()
documentation should be adjusted, regarding the support for the $callback_args
input type.
Currently it's documented as @param array
:
https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-admin/includes/template.php#L923
but as you noted, your WP User Profiles plugin is using an WP_User
object:
// Register the "Status" side meta box add_meta_box( 'submitdiv', _x( 'Status', 'users user-admin edit screen', 'wp-user-profiles' ), 'wp_user_profiles_status_metabox', $hook, 'side', 'high', $user );
#12
@
6 years ago
I’ve tweaked my WP User Profiles plugin to be an array in the next release. It’s also odd how it not being an array manifests itself once meta boxes are being output to the page.
Areas in WordPress core slyly avoid the error by having meta boxes without titles, or otherwise passing null in areas to trigger their own unique edge-cases.
Comments, Links, and Nav Menus all have their own little nuances, as does the Block Editor now. I don’t believe any of that logic belongs in do_meta_boxes()
but that’s another ticket for another day. :)
#13
@
6 years ago
- Keywords has-patch added
- Owner set to pento
- Status changed from new to assigned
Thanks for the bug report and the patch, @johnjamesjacoby!
I agree that we need to handle args
not being an array, this patch does the job nicely.
While I'm there, I'll also make the logic that @birgire referred to a little clearer. 🙂
45206.patch addresses the regression by wrapping the new Gutenberg-support code in a...
...condition. I think this is correct, as it only addresses the new code causing new fatal errors. Another potential fix might be to the
add_meta_box()
function, to change the$callback_args
default value fromnull
toarray()
, but I chose not to touch an unrelated function at this time to avoid introducing other unexpected errors elsewhere.Outside of this specific plugin incompatibility, I believe this patch is a nice-to-have bit of hardening anyways. Since
$box['args']
is not guaranteed to be an array, it makes sense to continue to check it in new code the same as do with old, at least until such a time where meta-box code can be scrutinized more deeply.