WordPress.org

Make WordPress Core

Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#7671 closed defect (bug) (fixed)

Plugin fatal error yields irrelevant error message from WordPress

Reported by: omry Owned by:
Milestone: 2.7 Priority: normal
Severity: normal Version: 2.7
Component: Plugins Keywords:
Focuses: Cc:

Description

The following plugin demonstrates the problem.
it dies with the error '123', but wp 2.6 outputs the following gem:

Fatal error: Cannot redeclare bogus_activate() (previously declared in /home/omry/dev/php/wp/2.6/wp-content/plugins/hello.php:13) in /home/omry/dev/php/wp/2.6/wp-content/plugins/hello.php on line 13
<?php
/*
Plugin Name: Bogus plugin
Plugin URI: bogus.org
Description: The bogus plugin from hell
Author: Ya
Version: 666.6
Author URI: nope.org
*/

$plugin_name = substr(__FILE__, strlen(ABSPATH . PLUGINDIR . '/'));
add_action("activate_$plugin_name",'bogus_activate');
function bogus_activate(){die('123');}


?>

Attachments (4)

7671.diff (1.0 KB) - added by DD32 13 years ago.
7671.2.diff (5.9 KB) - added by DD32 13 years ago.
7671.3.diff (581 bytes) - added by DD32 13 years ago.
catch_fatal_activation_hook.diff (752 bytes) - added by andy 11 years ago.

Download all attachments as: .zip

Change History (27)

#1 @DD32
13 years ago

  • Keywords reporter-feedback added

It works for me as expected, Can you provide clear and reproduceable steps? Ensuring you have no other plugins loaded which could affect it?

#2 @Viper007Bond
13 years ago

That error is also a PHP error, something WordPress has no control over.

#3 @DD32
13 years ago

  • Milestone 2.8 deleted
  • Resolution set to invalid
  • Status changed from new to closed
  • Version 2.6 deleted

Ahhh, Now i understand, Here was me thinking that the error of '123' was showing up and not wanted..

That PHP error was due to trying to activate an allready activated plugin, Due to you just using die(), WordPress marked it as activated, however never got to redirect the user to say that it had been activated.
The Workflow is like this:

Set the user to redirect to a failure-to-activate page
Include the plugin
<At this point, if theres a fatal error, it'll die and redirect>
Mark the plugin as activated
Run the activation hook
Redirect user to activated-ok page instead of failure page

If you hook in on the activation hook you'll break the workflow and end up in odd places.

Using deactivate_plugins() and wp_die can achieve what you're after, See this example plugin:

<?php
/*
Plugin Name: Newer WordPress Plugin
Plugin URI: http://dd32.id.au/wordpress-plugins/?plugin=newer-wordpress-test-plugin
Description: Tests non-activation. Requires WordPress 2.4 or later(deactivate_plugins() function call), Requires WordPress 2.8 or later to activate
Author: Dion Hulse
Version: 1.0
*/

register_activation_hook(__FILE__, 'nwp_act');

function nwp_act(){
	global $wp_version;
	if( version_compare( $wp_version, '2.7.9', '>=') )
		return; //Its WordPress 2.8 
	
	$message = '<h1>Newer WordPress Plugin</h1> <p>This plugin requires WordPress 2.8, which you do not have.</p> <p>Click <a href="plugins.php">here</a> to go back to plugins list</p>';

	if( function_exists('deactivate_plugins') )
		deactivate_plugins(__FILE__);
	else
		$message .= '<p><strong>Please deactivate this plugin Immediately</strong></p>'; //We couldnt automatically deactivate without messing with array, for Wordpress < 2.4
	wp_die($message);
}

?>

#4 @DD32
13 years ago

Thats odd, deactivate_plugins() within the activation hook doesnt appear to be working under trunk right now.. however, it did work under trunk(when trunk was 2.6). I'll look into why that doesnt appear to be working at some stage.

#5 @omry
13 years ago

any error inside the activated plugin generate some strange error messages.
wasn't the point of this detection mechanism to help point out the problem in broken plugins?

it's better not to provide an error message at all than to provide some bogus irrelevant message.

what if you swap those two:

Mark the plugin as activated
Run the activation hook

?

#6 @DD32
13 years ago

  • Milestone set to 2.7
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version set to 2.7

any error inside the activated plugin generate some strange error messages. wasn't the point of this detection mechanism to help point out the problem in broken plugins?

Yep, It was to protect against PHP errors, of which it currently does, die() or outputting some error message is not a Fatal PHP error which it was designed to protect against.

it's better not to provide an error message at all than to provide some bogus irrelevant message.

No bogus error is produced, The "Bogus" error you provided was a PHP Fatal error message about you trying to redefine a allready defined function, Which is due to your code killing execution when its not expected to.

what if you swap those two:

Then plugin which deactivate themselves on activation if requirements are not met will be broken.

I've found a mid-ground which i think makes a bit more sense, Put simply, If you die() within the activation function AND call deactivate_plugins(FILE); before doing so, It'll deactivate the plugin as expected, I'll attach a patch in a moment.

@DD32
13 years ago

#7 @omry
13 years ago

die was just an example of a problem with the plugin activation.

I am not sure what was the actual error in my plugin that caused the misleading error message to appear.

what I am really interested in is a way for a plugin to fail activation and to provide the user some error message.

naturally in such a case, the plugin should end up deactivated.
how would I achieve something like this?

#8 @DD32
13 years ago

it's better not to provide an error message at all than to provide some bogus irrelevant message.

No bogus error is produced, The "Bogus" error you provided was a PHP Fatal error message about you trying to redefine a allready defined function, Which is due to your code killing execution when its not expected to.

Just a rephrase there:

Its due to your plugin calling die() at a point AFTER which the fatal error protection is designed to protect.

Then plugin which deactivate themselves on activation if requirements are not met will be broken.

Along with the plugins which use the activation hook to spawn off on a "Configure this plugin wizard"-type setup, Which i cant think of one off the top of my head, but feel there is one.

what I am really interested in is a way for a plugin to fail activation and to provide the user some error message.
naturally in such a case, the plugin should end up deactivated. how would I achieve something like this?

function bogus_activate(){
deactivate_plugins(__FILE__);
die('123');
}

should work, once the attached patch is applied.

Under 2.6 that example code would not work as expected, It'd trigger an error, but not show the error.

In order to show the error, something like this would be used:

function bogus_activate(){
deactivate_plugins(__FILE__);
wp_die('123');
}

I apologise for my point being all over the place in the comments here, I'm probably dead-tired :)

#9 @omry
13 years ago

I understood your points just fine :).

I am fine with calling wp_die('123') if that will work globally (for present and future wp versions).

thanks.

#10 @ryan
13 years ago

  • Component changed from General to Plugins

@DD32
13 years ago

#11 @DD32
13 years ago

  • Keywords has-patch commit added; reporter-feedback removed

attachment 7671.2.diff added.

  • use activate_plugin for error scraping, Allows error hook to run, which deals better with plugins deactivating themselves
  • pass full WP_Error object to wp_die() since it can handle extracting the messages held within
  • Whitespace consistancy throughout the file

#12 @ryan
13 years ago

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

(In [9315]) use activate_plugin for error scraping, pass full WP_Error object to wp_die(), whitespace cleanups. Props DD32. fixes #7671

#13 @Speedboxer
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This patch seems to cause fatal errors not to show up at all (when I reverted the change, an error showed)...

@DD32
13 years ago

#14 @DD32
13 years ago

This patch seems to cause fatal errors not to show up at all

Indeed, Looks like something changd after my original patch was made.

attachment 7671.3.diff added.

  • Reverts use activate_plugin() as errors are silenced within
  • Runs the activate handle on error scrapes (as activate_plugin() no longer does it) - allows plugin activation code to kill activation

#15 @ryan
13 years ago

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

(In [9352]) Fix plugin activation error reporting. Props DD32. fixes #7671

#16 @andy
11 years ago

  • Keywords has-patch commit removed
  • Milestone 2.7 deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 2.7 to 3.0.1

Somewhere along the line this bug crept back in. If the script dies in the activate_$plugin hook, the plugin is activated and the message to the user is that a function has already been defined.

The fix is simple: run the activate_$plugin hook BEFORE saving the new active_plugins array.

#17 @mrmist
11 years ago

  • Version changed from 3.0.1 to 2.7

I could be speaking out of turn, but it's probably best not to re-open fixes that were complete on old milestones.

Also version tracks the original report generally.

#18 @scribu
11 years ago

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

(In [15744]) Fix irrelevant error message on plugin activation again. Props andy. Fixes #7671

#19 @scribu
11 years ago

mrmist, you're right. It's okay this time, since it's a straightforward fix to a regression.

#20 @andy
11 years ago

My apologies for the faux pas and my thanks for the commit.

#21 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#22 @nacin
11 years ago

  • Milestone set to 2.7
  • Resolution set to fixed
  • Status changed from reopened to closed

Because this belongs in the 3.0 branch I have opened #15062. If this were introduced in this cycle, reopening the ticket would be fine, but the list of tickets ina point release should IMO be reflective of all changes.

Don't mind me, just tacking on the original milestone here. Thanks Andy for finding this, as many of us have looked thanks to many reports of this in 3.0.

#23 @nacin
11 years ago

(In [15745]) Fix irrelevant error message on plugin activation again. Props andy. Fixes #15062 for the 3.0 branch. see #7671.

Note: See TracTickets for help on using tickets.