Skip to content
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

Core hooks fired in incorrect order, breaks CMB library #4929

Closed
johnbillion opened this issue Feb 7, 2018 · 10 comments
Closed

Core hooks fired in incorrect order, breaks CMB library #4929

johnbillion opened this issue Feb 7, 2018 · 10 comments
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Bug An existing feature does not function as intended

Comments

@johnbillion
Copy link
Member

johnbillion commented Feb 7, 2018

Issue Overview

The way in which Gutenberg loads meta boxes causes it to trigger core actions in the incorrect order on the post editing screen. One problem that this causes is to break meta boxes which are registered with the CMB library because the admin_enqueue_scripts action fires before the fields are registered, causing JavaScript files to not get enqueued.

Steps to Reproduce (for bugs)

  1. Install the Query Monitor plugin, or any debugging plugin which displays hooks in the order in which they are fired.
  2. Load the post editing screen without Gutenberg enabled and note that the dbx_post_advanced action fires before the admin_enqueue_scripts hook.
  3. Load the post editing screen with Gutenberg enabled and note that the order in which these two hooks fire is reversed.

Expected Behavior

The dbx_post_advanced hook fires before the admin_enqueue_scripts hook.

Current Behavior

The admin_enqueue_scripts hook fires before the dbx_post_advanced hook.

/cc @mikeselander @mattheu

@johnbillion johnbillion added [Type] Bug An existing feature does not function as intended Backwards Compatibility Issues or PRs that impact backwards compatability labels Feb 7, 2018
@johnbillion
Copy link
Member Author

johnbillion commented Feb 7, 2018

The underlying cause is that the gutenberg_collect_meta_box_data() function (which fires the dbx_post_advanced hook, among several others) fires on the admin_head action, which is too late.

@bobbingwide
Copy link
Contributor

bobbingwide commented Feb 7, 2018

Do you have a workaround, such as hooking into gutenberg_can_edit_post_type ?
See also #1316

@johnbillion
Copy link
Member Author

I don't have a workaround yet but I'm looking into it.

@johnbillion
Copy link
Member Author

I believe that the following is a workaround for this particular issue, but it's not been extensively tested. If anyone else has the same issue, feedback would be welcome.

add_action( 'admin_enqueue_scripts', function() {
	do_action( 'dbx_post_advanced', $GLOBALS['post'] );
	remove_all_actions( 'dbx_post_advanced' );
}, -999 );

@danielbachhuber danielbachhuber added this to the Merge Proposal milestone Apr 11, 2018
@karmatosed karmatosed modified the milestones: Merge Proposal, Merge Proposal: Back Compat Apr 12, 2018
@BE-Webdesign
Copy link
Contributor

The underlying cause is that the gutenberg_collect_meta_box_data() function (which fires the dbx_post_advanced hook, among several others) fires on the admin_head action, which is too late.

If it doesn't fire on admin_head it is too early for other meta box plugins, like ACF. For whatever reason a significant amount of ACF registrations happen on admin_head. I can look into this at some point, and fix it up for CMB. Don't have any bandwidth currently.

@danielbachhuber
Copy link
Member

From #7000 (comment)

NB: According to this comment, it looks like metaboxes are added "as soon as possible". I don't know why it cannot be done as soon as in the classic editor but worry that changing the order of two of the most widely used actions (See hooks usage statistics) could introduce other various backward compatibility issues in the ecosystem.

I think we may need to revert back to the existing order and have ACF include some form of compatibility shim.

@johnbillion
Copy link
Member Author

@jsternberg Hi! Hope you don't mind the ping. Are you aware if this issue affects CMB2?

@jsternberg
Copy link

Hi, I think you might have pinged the wrong person. I do not know what the acronym CMB2 stands for so I'm unlikely to know if this issue affects it.

@johnbillion
Copy link
Member Author

Sorry! @jtsternberg is who I was after :-)

@jtsternberg
Copy link

👋Looks like I'm too late as #10660 has resolved it. Sorry I missed the ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

8 participants