TGMPA / TGM-Plugin-Activation

TGM Plugin Activation is a PHP library that allows you to easily require or recommend plugins for your WordPress themes (and plugins). It allows your users to install, update and even automatically activate plugins in singular or bulk fashion using native WordPress classes, functions and interfaces. You can reference bundled plugins, plugins from the WordPress Plugin Repository or even plugins hosted elsewhere on the internet.
http://tgmpluginactivation.com/
GNU General Public License v2.0
1.76k stars 429 forks source link

Php error when auto-loading class prior to Wp Themes or Plugins initialization #488

Closed tlartaud closed 7 years ago

tlartaud commented 9 years ago

prevent error Call to undefined function did_action() in class-tgm-plugin-activation.php

https://www.google.fr/search?q=Call%20to%20undefined%20function%20did_action()%20in%20class-tgm-plugin-activation.php&rct=j

jrfnl commented 9 years ago

@rarst Would you mind reviewing this for me ? You're so much better at Composer ;-)

tlartaud commented 9 years ago

oh sorry, I didn't described at least anything. I apologize if i am misunderstanding something, but,

I have a project built on a WP stack which is including a composer.json. According to what I understand, the vendor/composer/autoload_files.php can be loaded inside a wp-plugin, inside a wp-theme, or prior to wp initialization where WP is just a composer dependency.

When both wp and tgmpa are dependencies, which can happen in some project, tgmpa class is loaded, but WP core functions are not yet defined.

jrfnl commented 9 years ago

@tlartaud Hi Thomas, no need to apologize. I understand the issue you are facing and appreciate very much that you are sending in a PR for this.

As I personally don't use Composer, I figured I'm not the best person to review this PR, aka my request for help to someone who I know is very well versed in Composer in combination with WP.

What strikes me as odd about the error is that autoload would load the files before WP. TGMPA should always be a dependency of a plugin or theme not of a WP install, as it won't do anything without a function in the plugin or theme providing it with the dependencies (and WP core does not contain such a function). So I'm wondering if this Composer setup is appropriate.

Autoloading the file when it's part of a vendor directory for a plugin or theme seems good to me, autoloading it as part of WP not so much. But then again, as I said, I don't use Composer, so I'd like the opinion of someone who knows more about this sort of setups.

Rarst commented 9 years ago

The file in question is mixing definitions with runtime code. It will fall apart regardless of how you call it, if it's done too early.

So this is architectural issue, not just autoload one. I am not confident what would make sense, not having used the library in practice. As far as I am concerned any Composer-enabled WP project should work as part of site stack, otherwise bulk of value Composer brings goes out of the window.

In general I would recommend to untangle definitions from runtime code, PSR-1 et al.

jrfnl commented 9 years ago

@Rarst Thanks for your input!

renegeuze commented 9 years ago

Accepting this code will break the plugin for people who use composer autoloading from inside functions.php or any other late loading file. Of course nobody should have done that but it was an available option for a long time so I'm sure somebody did.

Better solution is to change the entire structure(as @Rarst mentions) and bump up the version. For a backwards compatible fix, wrap parts inside function_exist. Also see: https://github.com/TGMPA/TGM-Plugin-Activation/issues/236#issuecomment-149845637

GaryJones commented 7 years ago

I've hit the same issue, when trying to run theme-level PHPCS or PHPUnit, on a theme that pulls in TGMPA via Composer. I load theme files by requiring vendor/autoload.php, so it naturally loads up the TGMPA class file too.

One neater solution, seems to be to add the following to the top of TGMPA:

if ( ! defined( 'ABSPATH' ) ) {
    return;
}

@Rarst @jrfnl Any drawbacks you can see with that?

jrfnl commented 7 years ago

@GaryJones There's a PR for that were I asked the same question: #594

GaryJones commented 7 years ago

594 is merged.