Update cmb2-conditionals.php #45
Conversation
Use CMB2 hooks instead of native WordPress hooks. No longer need to determine if CMB2 is loaded yet which resolves cases where CMB2 is loaded as part of a plugin that loads after this one. No longer to determine which page to load scripts on by piggy backing on CMB2 own logic.
Updated header so I can install this fork on my site with GitHub updater.
cmb2-conditionals.php
Outdated
| @@ -12,13 +12,13 @@ | |||
| * | |||
| * @wordpress-plugin | |||
| * Plugin Name: CMB2 Conditionals | |||
| * Plugin URI: https://github.com/jcchavezs/cmb2-conditionals | |||
| * Plugin URI: https://github.com/yaronguez/cmb2-conditionals | |||
There was a problem hiding this comment.
Please revert the change to the plugin URI.
There was a problem hiding this comment.
I performed that commit after making the pull request in order to use this fork on my own site in the meantime. I assumed it wouldn't be added to the request. I'll move it to a different branch instead.
cmb2-conditionals.php
Outdated
| * Description: Plugin to establish conditional relationships between fields in a CMB2 metabox. | ||
| * Author: José Carlos Chávez <[email protected]> | ||
| * Author URI: http://github.com/jcchavezs | ||
| * Github Plugin URI: https://github.com/jcchavezs/cmb2-conditionals | ||
| * Github Plugin URI: https://github.com/yaronguez/cmb2-conditionals |
There was a problem hiding this comment.
Please revert the change to the plugin URI.
There was a problem hiding this comment.
I performed that commit after making the pull request in order to use this fork on my own site in the meantime. I assumed it wouldn't be added to the request. I'll move it to a different branch instead.
cmb2-conditionals.php
Outdated
| * Github Branch: master | ||
| * Version: 1.0.4 | ||
| * Version: 1.0.5 |
There was a problem hiding this comment.
Version nr changes should be determined by the plugin owner, not by a random PR. Please revert.
There was a problem hiding this comment.
I performed that commit after making the pull request in order to use this fork on my own site in the meantime. I assumed it wouldn't be added to the request. I'll move it to a different branch instead.
| add_action( 'admin_init', array( $this, 'admin_init' ), self::PRIORITY ); | ||
| add_action( 'admin_footer', array( $this, 'admin_footer' ), self::PRIORITY ); | ||
| add_action( 'cmb2_admin_init', array( $this, 'admin_init' ), self::PRIORITY ); | ||
| add_action( 'cmb2_footer_enqueue', array( $this, 'admin_footer' ), self::PRIORITY ); | ||
|
|
There was a problem hiding this comment.
If I remember correctly (but I could well be wrong), these hooks weren't always available in CMB2.
So, since which version of CMB2 have they been available ? And does that mean there is a change in the minimum CMB2 requirement for this plugin if this change would be applied ?
If so, this will need to be annotated somewhere very clearly or else, this should become an if/else based on checking a CMB2 version constant to see if these hooks are available and if not, fall back to the old code.
There was a problem hiding this comment.
Looks like cmb2_admin_init was added August 30, 2015, during version 2.1.0. cmb2_footer_enqueue was added February 15, 2017 during version 2.2.3.1. The problem with checking the version of CMB2 before adding these hooks is that often times this plugin could be loaded before CMB is, hence the point of the fork to begin with. I'll clearly annotate the requirement and resubmit.
There was a problem hiding this comment.
I added the requirement to README.MD. Are there any other areas it should?
There was a problem hiding this comment.
Looks like cmb2_admin_init was added August 30, 2015, during version 2.1.0. cmb2_footer_enqueue was added February 15, 2017 during version 2.2.3.1.
For the cmb2_admin_init hook, I'm ok with moving up the minimum CMB2 requirement as that version is nearly two years old.
For the cmb2_footer_enqueue hook, I don't think that's a good idea as it's a too recent change, so I would advocate either not making the change or wrapping it in a version check.
The problem with checking the version of CMB2 before adding these hooks is that often times this plugin could be loaded before CMB is, hence the point of the fork to begin with. I'll clearly annotate the requirement and resubmit.
That's what the CMB2_LOADED check a couple of lines earlier is about and this should not be removed.
If you want to influence the running order of which plugin is loaded when, you should change the priority of the add_action( 'plugins_loaded', 'cmb2_conditionals_init' ); call in line 256. It now defaults to 10. Changing that to - for instance - 20 could already solve your issue.
If it doesn't, I would suggest adding an additional if/else within the conditional around that line to also check did_action( 'plugins_loaded' ).
if ( ( function_exists( 'wp_installing' ) && wp_installing() === false ) || ( ! function_exists( 'wp_installing' ) && ( ! defined( 'WP_INSTALLING' ) || WP_INSTALLING === false ) ) ) {
if ( did_action( 'plugins_loaded' ) > 0 ) {
cmb2_conditionals_init();
} else {
add_action( 'plugins_loaded', 'cmb2_conditionals_init' );
}
}Are there any other areas it should?
composer.json comes to mind... though a search of the files in the repo might turn up more places.
| @@ -100,10 +97,7 @@ public function __construct() { | |||
| * Decide whether to include the js-script or not. | |||
| */ | |||
| public function admin_footer() { | |||
| if ( ! in_array( $GLOBALS['pagenow'], array( 'post-new.php', 'post.php' ), true ) ) { | |||
There was a problem hiding this comment.
Why are you removing this ?
There was a problem hiding this comment.
No longer necessary. cmb2_footer_enqueue takes care of determining which page the JavaScript should load on. Alternatively, admin_enqueue_scripts is the correct way to add scripts to an admin page, checking the parameter to determine the page rather than admin_footer and checking $GLOBALS['pagenow']. See https://codex.wordpress.org/Plugin_API/Action_Reference/admin_enqueue_scripts
There was a problem hiding this comment.
If I remember correctly, there were reasons why this was done this way, not in the least as people apparently are also using this plugin on the front-end.
Please look at the git blame / git history to see why this is how it is before changing this.
Revert GitHub url and version bump for pull request. Will update on different branch.
|
Hi @yaronguez. Thanks for such a big effort. Are you willing to continue with this? |
Use CMB2 hooks instead of native WordPress hooks. No longer need to determine if CMB2 is loaded yet which resolves cases where CMB2 is loaded as part of a plugin that loads after this one. No longer to determine which page to load scripts on by piggy backing on CMB2 own logic.