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

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

Closed
wants to merge 2 commits into from

Conversation

tlartaud
Copy link

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
Copy link
Contributor

jrfnl commented Oct 13, 2015

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

@tlartaud
Copy link
Author

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
Copy link
Contributor

jrfnl commented Oct 13, 2015

@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
Copy link

Rarst commented Oct 15, 2015

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
Copy link
Contributor

jrfnl commented Oct 15, 2015

@Rarst Thanks for your input!

@renegeuze
Copy link

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: #236 (comment)

@GaryJones
Copy link
Member

GaryJones commented Apr 13, 2017

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
Copy link
Contributor

jrfnl commented Apr 13, 2017

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

@GaryJones
Copy link
Member

#594 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants