Ticket #6308 (new task)

Opened 8 months ago

Last modified 8 months ago

Plugin variable scope issue

Reported by: Denis-de-Bernardy Assigned to:
Priority: low Milestone: 2.9
Component: Administration Version: 2.5
Severity: minor Keywords: dev-feedback
Cc:

Description

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.

Attachments

menu.php-r7414.diff (1.0 kB) - added by neodude on 03/20/08 10:01:20.
checks if there are any plugins before adding plugin nav

Change History

03/20/08 09:59:47 changed by neodude

  • keywords set to has-patch.
  • owner changed from anonymous to neodude.
  • status changed from new to assigned.

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.

03/20/08 10:01:20 changed by neodude

  • attachment menu.php-r7414.diff added.

checks if there are any plugins before adding plugin nav

03/20/08 19:21:12 changed by Denis-de-Bernardy

  • keywords changed from has-patch to dev-feedback.
  • priority changed from normal to low.
  • type changed from defect to task.
  • severity changed from major to minor.
  • milestone deleted.

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.

03/20/08 19:22:39 changed by 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.

03/20/08 19:26:27 changed by Denis-de-Bernardy

  • summary changed from Edit Plugins screen issue to Plugin variable scope issue.

03/20/08 19:29:22 changed by lloydbudd

  • milestone set to 2.6.

Not sure why you removed the milestone Denis.

03/21/08 02:03:46 changed by neodude

  • owner deleted.
  • status changed from assigned to new.

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.

03/21/08 11:48:05 changed by 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. :)