Plugin fatal error yields irrelevant error message from WordPress

[omry]

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');}


?>

[DD32]

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

[Viper007Bond]

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

[DD32]

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);
}

?>

[DD32]

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.

[omry]

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

?

[DD32]

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.

[omry]

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?

[DD32]

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

[omry]

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.

[DD32]

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

[ryan]

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

[Speedboxer]

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

[DD32]

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

[ryan]

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

[andy]

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.

[mrmist]

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.

[scribu]

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

[scribu]

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

[andy]

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

[nacin]

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.

[nacin]

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