Plugin variable scope issue

[Denis-de-Bernardy]

In WP 2.5 RC1, from the Plugins screen, when I visit the Edit Plugins screen, it leads me to:

/wp-admin/plugin-editor.php

And I'm greeted with a screen that displays but an error message:

"Sorry, that file cannot be edited."

Presumably, because an argument is missing in the url. Or, because it scans for a writable file and fails to locate one.

If the latter case, WP should check if any files are writable before even showing the menu item, or it fail in a more gentle manner.

[neodude]

I was able to reproduce this by removing all plugins (i.e. dolly and akismet from a trunk checkout).

I think the attached patch would be the least-code-changed solution, but perhaps it is expensive to get an array of all plugins on every invocation of every admin page?

Alternatively, the same "no plugins" error could be taken from plugins.php into plugin-editor.php.

[neodude]

checks if there are any plugins before adding plugin nav

[Denis-de-Bernardy]

Imho, scanning through the entire list of plugins on every page of the admin area is a rather bad idea. At most, it should be done when we're browsing the plugins menu.

Btw, further investigation revealed it was actually one of my plugins that was causing the error. I had a loop like this in a plugin file:

foreach ( array('file.php') as file )
include $path . $file;

Upon deactivating it, I could access the plugin editor page again.

This prompts me to a thought, however. My understanding was that, in WP 2.5, plugin files now needed to explicitly declare variables in the global scope. So I felt rather save to use the $file variable... Reading through the wp-settings.php file reveals they aren't.

Anyway, I can see all sorts of false alarms like this one happening because of this because of plugins overwriting global variables without giving thoughts to how WP might be using them later on. And I'd strongly suggest to give that idea a thought once again if it has been dropped.

The fix is rather simple. Simply change:

if ( get_option('active_plugins') ) {
	$current_plugins = get_option('active_plugins');
	if ( is_array($current_plugins) ) {
		foreach ($current_plugins as $plugin) {
			if ('' != $plugin && file_exists(ABSPATH . PLUGINDIR . '/' . $plugin))
				include_once(ABSPATH . PLUGINDIR . '/' . $plugin);
		}
	}
}

To:

if ( $current_plugins = get_option('active_plugins') && is_array($current_plugins ) {
	function wp_include_plugin($plugin) {
		if ('' != $plugin && file_exists(ABSPATH . PLUGINDIR . '/' . $plugin))
			include_once(ABSPATH . PLUGINDIR . '/' . $plugin);
	}
	
	foreach ($current_plugins as $plugin) {
		wp_include_plugin($plugin);
	}
}

It mostly goes down to weighing the costs of breaking a few plugins vs the costs of seeing an invalid bug every now and then.

[Denis-de-Bernardy]

mmm, works better with parenthesis :)

if ( ( $current_plugins = get_option('active_plugins') ) && is_array($current_plugins ) {
	function wp_include_plugin($plugin) {
		if ('' != $plugin && file_exists(ABSPATH . PLUGINDIR . '/' . $plugin))
			include_once(ABSPATH . PLUGINDIR . '/' . $plugin);
	}
	
	foreach ($current_plugins as $plugin) {
		wp_include_plugin($plugin);
	}
}

but anyway... I'd be curious to get Ryan's thoughts on this.

[lloydbudd]

Not sure why you removed the milestone Denis.

[neodude]

Denis - you seemed to have it covered then? I understood the problem differently and hence fixed a different bug - and a minor, minor one at that.

[Denis-de-Bernardy]

@neodude: yeah, the bug was indeed a different one

@lloydbudd: because I had initially set it to wp 2.5, thinking the bug was very real, major on top of that, and addressed immediately. given the true nature of the bug, a core dev would know better if and when this should be addressed. :)

[DD32]

perhaps adding a unset($plugin); after the plugin loading code could fix this, I've seen the $plugin variable pop through a few times with the last-loaded plugin, Confused me a few times..

[DD32]

Plugins shouldnt be setting generically named global variables, nor executing code (aside from Plugin API functions & initiating classes) in global scope.

attachment 6308.diff added

• Cleans up the $plugin variable after the plugins are included, Just for safty precautions for anything else which uses the $plugin variable in global scope elsewhere.
• some extra spacing

I feel that this would probably suffice for this ticket. Leaving the rest up to plugin devs best practive (of avoiding global variables in PHP)

[westi]

Replying to DD32:

Plugins shouldnt be setting generically named global variables, nor executing code (aside from Plugin API functions & initiating classes) in global scope.

Agreed.

attachment 6308.diff added

• Cleans up the $plugin variable after the plugins are included, Just for safty precautions for anything else which uses the $plugin variable in global scope elsewhere.
• some extra spacing

I feel that this would probably suffice for this ticket. Leaving the rest up to plugin devs best practive (of avoiding global variables in PHP)

Looks good to me.

[westi]

(In [10624]) Unset once we have included all the plugins to keep things tidy. Fixes #6308 props DD32.