WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#6308 closed defect (bug) (fixed)

Plugin variable scope issue

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.8 Priority: low
Severity: minor Version: 2.5
Component: Administration Keywords: dev-feedback has-patch commit
Focuses: 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 (2)

menu.php-r7414.diff (1011 bytes) - added by neodude 7 years ago.
checks if there are any plugins before adding plugin nav
6308.diff (915 bytes) - added by DD32 6 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 @neodude7 years ago

  • Keywords has-patch added
  • 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.

@neodude7 years ago

checks if there are any plugins before adding plugin nav

comment:2 @Denis-de-Bernardy7 years ago

  • Keywords dev-feedback added; has-patch removed
  • Milestone 2.5 deleted
  • Priority changed from normal to low
  • Severity changed from major to minor
  • Type changed from defect to task

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.

comment:3 @Denis-de-Bernardy7 years ago

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.

comment:4 @Denis-de-Bernardy7 years ago

  • Summary changed from Edit Plugins screen issue to Plugin variable scope issue

comment:5 @lloydbudd7 years ago

  • Milestone set to 2.6

Not sure why you removed the milestone Denis.

comment:6 @neodude7 years ago

  • Owner neodude 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.

comment:7 @Denis-de-Bernardy7 years ago

@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. :)

comment:8 @DD327 years ago

  • Type changed from task (blessed) to defect (bug)

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..

comment:9 @mrmist7 years ago

  • Keywords needs-patch added

@DD326 years ago

comment:10 follow-up: @DD326 years ago

  • Keywords has-patch commit added; needs-patch removed
  • Milestone changed from 2.9 to 2.8

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)

comment:11 in reply to: ↑ 10 @westi6 years ago

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.

comment:12 @westi6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.