WPupdatePHP / wp-update-php

Library to be bundled with WordPress plugins to enforce users to upgrade to supported PHP versions.
GNU General Public License v2.0
79 stars 11 forks source link

Wrap in a `class_exists` to avoid fatals #6

Closed johnbillion closed 9 years ago

johnbillion commented 9 years ago

If two plugins both include this class, then you have a bigger problem than minimum PHP versions. The class should be wrapped in a class_exists( 'WPUpdatePhp' ) check.

coenjacobs commented 9 years ago

Yep, I agree. I think it's the easiest to just have something like

if ( class_exists( 'WPUpdatePhp' ) ) {
return;
}

at the top of the class file, so the implementing plugin doesn't need to look after this?

afragen commented 9 years ago

Revised to #14

coenjacobs commented 9 years ago

This is closed via https://github.com/WPupdatePHP/wp-update-php/commit/d29f0eef40bc7f0ced07160537abe0dab403ba8e and will be available in the 1.1.0 release.

pderksen commented 9 years ago

@coenjacobs Please consider before the next release.

if ( class_exists( 'WPUpdatePhp' ) ) {
  return;
}

The above works in my tests involving 2 plugins that include the same library, but I had several customers get the error message "Fatal error: Cannot redeclare class WPUpdatePhp" with the same 2 plugins.

Not sure why I couldn't repeat it myself, but I think this check isn't stable enough.

With other similar class checks such as my plugins using the Stripe PHP library or EDD software licensing, I elect to do it like this from the init code:

if ( ! class_exists( 'WPUpdatePhp' ) ) {
    require_once( 'libraries/WPUpdatePhp.php' );
}

or simply wrap the whole class in a class_exists check above the class file (instead of using return):

if ( ! class_exists( 'WPUpdatePhp' ) ) {
    class WPUpdatePhp {
        /* Class code here... */
    }
}

I'm fine by either method.

coenjacobs commented 9 years ago

The above works in my tests involving 2 plugins that include the same library, but I had several customers get the error message "Fatal error: Cannot redeclare class WPUpdatePhp" with the same 2 plugins.

Are you sure both have the class_exists() check at the top of the library file in both plugins? If one of them doesn't have this, the error can occur already. Wrapping the library file in an if statement is definitely something that you can do, but shouldn't be required. I can add it to the documentation, but would love some more feedback from you since you've obviously found something going wrong. :smile:

pderksen commented 9 years ago

Yes it's at the top of both. I'm using an unmodified version of WPUpdatePhp.php from the release-1-1-0 branch as of March 17, 2015.

The 2nd plugin was an add-on to the first, so I wound up removing WPUpdatePhp from it since we add warnings that the first/base plugin needs to be active to run the add-on anyway.

Feel free to close this unless the issue pops up for others. Up to you on adding to the documentation or not at this point.

Thanks for chiming in!

keesiemeijer commented 9 years ago

You can't reliably check if the class exists because the class_exist check wasn't used from the beginning. If a plugin has the 1.1.0 version (with class_exist) and is loaded before another plugin with the old version you will still get a fatal error. This problem is more difficult than it seems at first glance.

The usage example on the GitHub page should also be wrapped in a class_exist. Users will blame the plugin being activated for the fatal error, even if it is using version 1.1.0 and checks if the class exists before instantiating.

coenjacobs commented 9 years ago

Yep, I agree. The example implementation should contain the class_exists check and this should be dropped from the class itself. class_exists checks make no sense in files like that.