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.

Sign up for GitHub

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

Update cmb2-conditionals.php #45

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update cmb2-conditionals.php #45

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19c8ebc
Update cmb2-conditionals.php
yaronguez Jul 11, 2017
20d5683
Update cmb2-conditionals.php
yaronguez Jul 11, 2017
92caeb8
Update cmb2-conditionals.php
yaronguez Jul 11, 2017
7f3a83c
Update README.md
yaronguez Jul 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions cmb2-conditionals.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the change to the plugin URI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the change to the plugin URI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

* Github Branch: master
* Version: 1.0.4
* Version: 1.0.5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version nr changes should be determined by the plugin owner, not by a random PR. Please revert.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

* License: GPL v3
*
* Copyright (C) 2015, José Carlos Chávez - [email protected]
Expand Down Expand Up @@ -83,13 +83,10 @@ class CMB2_Conditionals {
/**
* Constructor - Set up the actions for the plugin.
*/
public function __construct() {
if ( ! defined( 'CMB2_LOADED' ) || false === CMB2_LOADED ) {
return;
}
public function __construct() {

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the requirement to README.MD. Are there any other areas it should?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

foreach ( $this->maybe_required_form_elms as $element ) {
add_filter( "cmb2_{$element}_attributes", array( $this, 'maybe_set_required_attribute' ), self::PRIORITY );
Expand All @@ -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 ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing this ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}


wp_enqueue_script(
'cmb2-conditionals',
plugins_url( '/cmb2-conditionals.js', __FILE__ ),
Expand Down