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

Meta Box back compat and fall backs #3554

Merged
merged 14 commits into from Nov 27, 2017

Conversation

@pento
Member

pento commented Nov 20, 2017

Even after Gutenberg is merged into Core, and a plugin has upgraded all of its meta boxes to using Gutenberg APIs, those plugin will still need to register classic meta boxes, for backwards compatibility with the classic editor.

Changes

This PR makes a few changes along those lines.

Back Compat Meta Boxen

When registering a meta box, adding a '__back_compat_meta_box' => true arg to the $callback_args argument of add_meta_box() will cause Gutenberg to ignore that meta box in the meta box UI - it's assumed that a plugin developer who defines this arg will have registered a Gutenberg-specific UI for this meta box.

For example:

add_meta_box(
	'lol',
	'foo',
	function() {
		echo 'whee';
	},
	null,
	'normal',
	'high',
	array(
		'__back_compat_meta_box' => true,
		'__block_editor_compatible_meta_box' => true,
	)
);

Falling Back To The Classic Editor

If an unsupported meta box is detected while loading Gutenberg, we now redirect to the classic editor. Currently, this is any meta box that explicitly declares the __block_editor_compatible_meta_box arg as a false-y value.

A future iteration could check if a meta box relies on complex scripts which are unlikely to work in Gutenberg, and fall back.

Warning the End User

Ultimately, the end user should know when a plugin causes a fallback to the classic editor. The warning looks something like this:

Screenshot of the meta box fallback warning

This currently only shows when WP_DEBUG is enabled, though would eventually be enabled at all times.

It doesn't show for meta boxes that set the __back_compat_meta_box flag, and naturally wouldn't show for other meta boxes that don't trigger a fallback.

Screenshot of a meta box without the fallback warning

Code Note

Obviously, some of this code is kind of weird, and a little bit hacky, to hook into the existing meta box rendering scheme. gutenberg_intercept_meta_box_render() and gutenberg_override_meta_box_callback(), for example, will be reduced to some small changes to do_meta_boxes(), come merge time.

#952

pento added some commits Nov 20, 2017

When a meta box is registered that doesn't declare itself compatible …
…with Gutenberg, fall back to the classic editor.

@pento pento added this to the Beta 1.8 milestone Nov 20, 2017

@pento pento self-assigned this Nov 20, 2017

@pento pento requested review from youknowriad, mtias, aduth and BE-Webdesign Nov 23, 2017

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Nov 23, 2017

Member

This is ready for merge, we can iterate on it more as we determine reasons for fallback.

Member

pento commented Nov 23, 2017

This is ready for merge, we can iterate on it more as we determine reasons for fallback.

@@ -36,7 +70,7 @@ have to do, unless we want to move `createEditorInstance()` to fire in the foote
or at some point after `admin_head`. With recent changes to editor bootstrapping
this might now be possible. Test with ACF to make sure.
### Redux and React Meta Box Management.
### Redux and React Meta Box Management
*The Redux store by default will hold all meta boxes as inactive*. When
`INITIALIZE_META_BOX_STATE` comes in, the store will update any active meta box

This comment has been minimized.

@youknowriad

youknowriad Nov 24, 2017

Contributor

Unrelated but we should add this file to the Gutenberg Handbook.

@youknowriad

youknowriad Nov 24, 2017

Contributor

Unrelated but we should add this file to the Gutenberg Handbook.

This comment has been minimized.

@pento

pento Nov 27, 2017

Member

Good point, I didn't even notice that it wasn't there. 🙂 Added!

@pento

pento Nov 27, 2017

Member

Good point, I didn't even notice that it wasn't there. 🙂 Added!

foreach ( $contexts as $context => $priorities ) {
foreach ( $priorities as $priority => $boxes ) {
foreach ( $boxes as $id => $box ) {
if ( ! is_array( $wp_meta_boxes[ $post_type ][ $context ][ $priority ][ $id ]['args'] ) ) {

This comment has been minimized.

@youknowriad

youknowriad Nov 24, 2017

Contributor

Can we replace $wp_meta_boxes[ $post_type ][ $context ][ $priority ][ $id ] by $box in this loop?

@youknowriad

youknowriad Nov 24, 2017

Contributor

Can we replace $wp_meta_boxes[ $post_type ][ $context ][ $priority ][ $id ] by $box in this loop?

This comment has been minimized.

@pento

pento Nov 27, 2017

Member

We can really only replace the first instance - $box is a copy of the array element, so modifications need to be applied to $wp_meta_boxes.

@pento

pento Nov 27, 2017

Member

We can really only replace the first instance - $box is a copy of the array element, so modifications need to be applied to $wp_meta_boxes.

This comment has been minimized.

@youknowriad

youknowriad Nov 27, 2017

Contributor

Too many JS in my head :)

@youknowriad

youknowriad Nov 27, 2017

Contributor

Too many JS in my head :)

if ( isset( $box['args']['__back_compat_meta_box'] ) ) {
$block_compatible |= (bool) $box['args']['__back_compat_meta_box'];
unset( $box['args']['__back_compat_meta_box'] );

This comment has been minimized.

@youknowriad

youknowriad Nov 24, 2017

Contributor

Just per curiosity, why do we unset these keys?

@youknowriad

youknowriad Nov 24, 2017

Contributor

Just per curiosity, why do we unset these keys?

This comment has been minimized.

@pento

pento Nov 27, 2017

Member

Copying the same pattern from Core - the meta box render function doesn't need to know about these keys.

@pento

pento Nov 27, 2017

Member

Copying the same pattern from Core - the meta box render function doesn't need to know about these keys.

*/
function gutenberg_show_meta_box_warning( $callback ) {
// Only show the warning when WP_DEBUG is enabled.
if ( ! WP_DEBUG ) {

This comment has been minimized.

@youknowriad

youknowriad Nov 24, 2017

Contributor

I wonder if we should not show it even in "prod" while we're on the plugin phase and introduce this change later. To encourage people to check their metaboxes?

@youknowriad

youknowriad Nov 24, 2017

Contributor

I wonder if we should not show it even in "prod" while we're on the plugin phase and introduce this change later. To encourage people to check their metaboxes?

This comment has been minimized.

@pento

pento Nov 27, 2017

Member

I did this way while we're still working out the path for plugin devs to move their meta boxes to blocks - it doesn't seem fair to show a warning that the dev can't do anything to fix.

Once there are well documented upgrade paths, we can take away this check. Once it comes time to merge, we can evaluate how we want to merge it. It might even be valuable to have a "plugin x caused this fall back" message in prod, so users know what happened, and a "here are links to documentation and tutorials to upgrade your meta boxes" when WP_DEBUG is set, so plugin devs have the information they need.

@pento

pento Nov 27, 2017

Member

I did this way while we're still working out the path for plugin devs to move their meta boxes to blocks - it doesn't seem fair to show a warning that the dev can't do anything to fix.

Once there are well documented upgrade paths, we can take away this check. Once it comes time to merge, we can evaluate how we want to merge it. It might even be valuable to have a "plugin x caused this fall back" message in prod, so users know what happened, and a "here are links to documentation and tutorials to upgrade your meta boxes" when WP_DEBUG is set, so plugin devs have the information they need.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 24, 2017

Contributor

On my testing I have ACF with some fields, I was expecting Gutenberg to redirect because by default all metaboxes are not compatible with gutenberg yet? I'm I wrong?

Also, I noticed this warning

screen shot 2017-11-24 at 09 28 03

The warning is hidden behind the Gutenberg UI (make sure to check the HTML output)

Contributor

youknowriad commented Nov 24, 2017

On my testing I have ACF with some fields, I was expecting Gutenberg to redirect because by default all metaboxes are not compatible with gutenberg yet? I'm I wrong?

Also, I noticed this warning

screen shot 2017-11-24 at 09 28 03

The warning is hidden behind the Gutenberg UI (make sure to check the HTML output)

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Nov 27, 2017

Member

On my testing I have ACF with some fields, I was expecting Gutenberg to redirect because by default all metaboxes are not compatible with gutenberg yet? I'm I wrong?

It currently only redirects if a meta box declares itself incompatible with Gutenberg - the idea is to have an opt-out system, combined with potentially detecting meta boxen that are incompatible.

Also, I noticed this warning

Nice catch, fixed!

Member

pento commented Nov 27, 2017

On my testing I have ACF with some fields, I was expecting Gutenberg to redirect because by default all metaboxes are not compatible with gutenberg yet? I'm I wrong?

It currently only redirects if a meta box declares itself incompatible with Gutenberg - the idea is to have an opt-out system, combined with potentially detecting meta boxen that are incompatible.

Also, I noticed this warning

Nice catch, fixed!

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 27, 2017

Codecov Report

Merging #3554 into master will increase coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3554      +/-   ##
==========================================
+ Coverage    34.9%   36.03%   +1.13%     
==========================================
  Files         263      324      +61     
  Lines        6727     8453    +1726     
  Branches     1227     1571     +344     
==========================================
+ Hits         2348     3046     +698     
- Misses       3694     4564     +870     
- Partials      685      843     +158
Impacted Files Coverage Δ
blocks/library/shortcode/index.js 33.33% <0%> (-6.67%) ⬇️
blocks/library/heading/index.js 21.62% <0%> (-3.38%) ⬇️
blocks/library/paragraph/index.js 25.8% <0%> (-3.37%) ⬇️
blocks/api/validation.js 93.54% <0%> (-1.62%) ⬇️
blocks/library/html/index.js 15.78% <0%> (-0.88%) ⬇️
blocks/library/more/index.js 21.42% <0%> (-0.8%) ⬇️
blocks/library/list/index.js 6.2% <0%> (-0.7%) ⬇️
editor/components/inserter/menu.js 86.88% <0%> (-0.44%) ⬇️
editor/selectors.js 92.83% <0%> (-0.29%) ⬇️
components/popover/index.js 84.61% <0%> (-0.15%) ⬇️
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1709207...0b22b58. Read the comment docs.

codecov bot commented Nov 27, 2017

Codecov Report

Merging #3554 into master will increase coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3554      +/-   ##
==========================================
+ Coverage    34.9%   36.03%   +1.13%     
==========================================
  Files         263      324      +61     
  Lines        6727     8453    +1726     
  Branches     1227     1571     +344     
==========================================
+ Hits         2348     3046     +698     
- Misses       3694     4564     +870     
- Partials      685      843     +158
Impacted Files Coverage Δ
blocks/library/shortcode/index.js 33.33% <0%> (-6.67%) ⬇️
blocks/library/heading/index.js 21.62% <0%> (-3.38%) ⬇️
blocks/library/paragraph/index.js 25.8% <0%> (-3.37%) ⬇️
blocks/api/validation.js 93.54% <0%> (-1.62%) ⬇️
blocks/library/html/index.js 15.78% <0%> (-0.88%) ⬇️
blocks/library/more/index.js 21.42% <0%> (-0.8%) ⬇️
blocks/library/list/index.js 6.2% <0%> (-0.7%) ⬇️
editor/components/inserter/menu.js 86.88% <0%> (-0.44%) ⬇️
editor/selectors.js 92.83% <0%> (-0.29%) ⬇️
components/popover/index.js 84.61% <0%> (-0.15%) ⬇️
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1709207...0b22b58. Read the comment docs.

@youknowriad

Thanks for working on this. Let's get this in. I personally prefer it to be opt-in by let's try this and see if we should revisit.

@pento pento merged commit 9864a60 into master Nov 27, 2017

3 checks passed

codecov/project 36.03% (+1.13%) compared to 1709207
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@pento pento deleted the add/meta-box-fallback branch Nov 27, 2017

```php
add_meta_box( 'my-meta-box', 'My Meta Box', 'my_meta_box_callback',
null, 'normal', 'high',

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Mixed whitespace, tabs and spaces.

@aduth

aduth Nov 27, 2017

Member

Mixed whitespace, tabs and spaces.

This comment has been minimized.

@pento

pento Nov 28, 2017

Member

Noice. #3684.

@pento

pento Nov 28, 2017

Member

Noice. #3684.

}
// Incompatible meta boxes require an immediate redirect to the classic editor.
if ( $incompatible_meta_box ) {

This comment has been minimized.

@aduth

aduth Nov 27, 2017

Member

Could we not perform the redirect server-side here?

@aduth

aduth Nov 27, 2017

Member

Could we not perform the redirect server-side here?

This comment has been minimized.

@pento

pento Nov 28, 2017

Member

With the current code, no. gutenberg_collect_meta_box_data() happens in the admin_head action, well after HTML has started being sent.

@pento

pento Nov 28, 2017

Member

With the current code, no. gutenberg_collect_meta_box_data() happens in the admin_head action, well after HTML has started being sent.

@pento pento referenced this pull request Nov 29, 2017

Closed

Supporting Metaboxes #952

jeremyfelt added a commit to jeremyfelt/gutenberg that referenced this pull request Dec 7, 2017

Don't intercept rendering of removed meta boxes
When a metabox is removed with `remove_metabox()`, the meta box
array is updated as:

`$wp_meta_boxes[$page][$context][$priority][$id] = false;`

When `gutenberg_intercept_meta_box_render()` loops through each of
these contexts, priorities, and boxes, it rebuilds the previously
removed metabox with partial information.

Code like `do_meta_boxes()` then loops through and can generate
a notice when `$box` is no longer falsy and `$box['title']` is not
available.

Introduced in WordPress#3554.

@jeremyfelt jeremyfelt referenced this pull request Dec 7, 2017

Merged

Don't intercept rendering of removed meta boxes #3856

3 of 3 tasks complete
@@ -219,10 +219,40 @@ function gutenberg_collect_meta_box_data() {
// If the meta box should be empty set to false.
foreach ( $locations as $location ) {
if ( isset( $_meta_boxes_copy[ $post->post_type ][ $location ] ) && gutenberg_is_meta_box_empty( $_meta_boxes_copy, $location, $post->post_type ) ) {
if ( gutenberg_is_meta_box_empty( $_meta_boxes_copy, $location, $post->post_type ) ) {

This comment has been minimized.

@youknowriad

youknowriad Jan 12, 2018

Contributor

This call is always returning false for "side" meta boxes because the array doesn't have a "side" key even if I do have a side meta box? Any idea how to fix this @pento

@youknowriad

youknowriad Jan 12, 2018

Contributor

This call is always returning false for "side" meta boxes because the array doesn't have a "side" key even if I do have a side meta box? Any idea how to fix this @pento

This comment has been minimized.

@youknowriad

youknowriad Jan 25, 2018

Contributor

Nevermind, I might have had a weird setup at that moment. It's working as expected right now.

@youknowriad

youknowriad Jan 25, 2018

Contributor

Nevermind, I might have had a weird setup at that moment. It's working as expected right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment