Logic error in do_meta_boxes() for Gutenberg support

[johnjamesjacoby]

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.

[johnjamesjacoby]

45206.patch addresses the regression by wrapping the new Gutenberg-support code in a...

if ( is_array( $box[ 'args' ] ) ) { ... }

...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 from null to array(), 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.

[birgire]

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?

Related to #45112, and r43608.

Also this changeset reference seems to be cron related and seems unrelated?

[johnjamesjacoby]

@birgire

1. Not a typo in the patch. That's the original code and is a bitwise comparison in PHP.
2. I forget that Trac auto links to core and not develop. Sorry about that. Will fix.

[birgire]

@johnjamesjacoby thanks updating the changeset references and thanks for the clarification, I totally missed that this comparison came from original code :-)

[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.

[birgire]

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:

​https://plugins.trac.wordpress.org/browser/wp-user-profiles/trunk/wp-user-profiles/includes/metaboxes.php#L51

// 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
);

[johnjamesjacoby]

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. :)

[pento]

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. 🙂

[pento]

In 43838:

Meta boxes: Don't assume that callback args are an array.

While the documentation for add_meta_box() specifices that $callback_args should be an array, this has never been enforced, and we have workarounds in place for when it's passed as something other than an array.

Rather than break sites that are passing unexpected data, we can quietly just allow for it, instead.

Props johnjamesjacoby, birgire.
Fixes #45206.

[SergeyBiryukov]

Reopening for trunk.

[atimmer]

In 44174:

Meta boxes: Don't assume that callback args are an array.

While the documentation for add_meta_box() specifices that $callback_args should be an array, this has never been enforced, and we have workarounds in place for when it's passed as something other than an array.

Rather than break sites that are passing unexpected data, we can quietly just allow for it, instead.

Props johnjamesjacoby, birgire, pento.
Merges [43838] to trunk.
Fixes #45206.