-
Notifications
You must be signed in to change notification settings - Fork 277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066
Changes from 25 commits
6fed4da
2ca6335
2f45f25
3d69181
d1ee3c9
1747574
75cd7b4
a46d412
7204067
7aa4329
aaf683d
d255141
d351a63
cf2a580
a3eccc6
8627934
fd4cb60
7414381
2dd659e
9b148b2
622099d
d7e7827
56cd875
2efc8e2
dde935a
c0dd231
0c09fc0
e27c6fb
bb24b49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,28 +25,83 @@ describe('Blueprint step activatePlugin()', () => { | |
php = await handler.getPrimaryPhp(); | ||
}); | ||
|
||
it('should activate the plugin', async () => { | ||
const docroot = php.documentRoot; | ||
it('should activate a plugin file located in the plugins directory', async () => { | ||
const docroot = handler.documentRoot; | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
); | ||
await activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}); | ||
|
||
const response = await php.run({ | ||
code: `<?php | ||
require_once '/wordpress/wp-load.php'; | ||
require_once ${phpVar(docroot)}. "/wp-admin/includes/plugin.php" ; | ||
echo is_plugin_active('test-plugin.php') ? 'true' : 'false'; | ||
`, | ||
}); | ||
expect(response.text).toBe('true'); | ||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
|
||
it('should activate a plugin file located in a subdirectory of the plugins directory', async () => { | ||
const docroot = handler.documentRoot; | ||
const pluginDir = `${docroot}/wp-content/plugins/test-plugin`; | ||
php.mkdir(pluginDir); | ||
php.writeFile( | ||
`${pluginDir}/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
); | ||
|
||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: `test-plugin/test-plugin.php`, | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
|
||
it('should activate a plugin if a absolute plugin path is provided', async () => { | ||
const docroot = handler.documentRoot; | ||
php.mkdir(`${docroot}/wp-content/plugins/test-plugin`); | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin/index.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
); | ||
|
||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: `${docroot}/wp-content/plugins/test-plugin/index.php`, | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
|
||
it('should activate a plugin if a absolute plugin directory path is provided', async () => { | ||
const docroot = handler.documentRoot; | ||
php.mkdir(`${docroot}/wp-content/plugins/test-plugin`); | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
); | ||
|
||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: `${docroot}/wp-content/plugins/test-plugin`, | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
|
||
it('should activate a plugin if a absolute plugin directory path with a trailing slash is provided', async () => { | ||
const docroot = handler.documentRoot; | ||
php.mkdir(`${docroot}/wp-content/plugins/test-plugin`); | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
); | ||
|
||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: `${docroot}/wp-content/plugins/test-plugin/`, | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
|
||
it('should detect a silent failure in activating the plugin', async () => { | ||
const docroot = php.documentRoot; | ||
const docroot = handler.documentRoot; | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
|
@@ -56,18 +111,17 @@ describe('Blueprint step activatePlugin()', () => { | |
`${docroot}/wp-content/mu-plugins/0-exit.php`, | ||
`<?php exit(0); ` | ||
); | ||
expect( | ||
async () => | ||
await activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}) | ||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}) | ||
).rejects.toThrow(/Plugin test-plugin.php could not be activated/); | ||
}); | ||
|
||
it('should run the activation hooks as a priviliged user', async () => { | ||
const docroot = php.documentRoot; | ||
it('should run the activation hooks as a privileged user', async () => { | ||
const docroot = handler.documentRoot; | ||
const createdFilePath = | ||
docroot + '/activation-ran-as-a-priviliged-user.txt'; | ||
docroot + '/activation-ran-as-a-privileged-user.txt'; | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */ | ||
|
@@ -84,4 +138,61 @@ describe('Blueprint step activatePlugin()', () => { | |
|
||
expect(php.fileExists(createdFilePath)).toBe(true); | ||
}); | ||
|
||
it('should activate a plugin if it redirects during activation', async () => { | ||
const docroot = handler.documentRoot; | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin.php`, | ||
`<?php | ||
/** | ||
* Plugin Name: Test Plugin | ||
*/ | ||
add_action( 'activated_plugin', function( $plugin ) { | ||
if( $plugin == plugin_basename( __FILE__ ) ) { | ||
wp_redirect( admin_url( 'edit.php' ) ); | ||
exit(); | ||
} | ||
} ); | ||
` | ||
); | ||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
|
||
it('should activate a plugin if it produces a output during activation', async () => { | ||
const docroot = handler.documentRoot; | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin.php`, | ||
`<?php | ||
/** | ||
* Plugin Name: Test Plugin | ||
*/ | ||
echo 'Hello World'; | ||
` | ||
); | ||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}) | ||
).resolves.toBeUndefined(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return values doesn't seem relevant here, what we're really after is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done c0dd231 |
||
}); | ||
|
||
it('should not throw an error if the plugin is already active', async () => { | ||
const docroot = handler.documentRoot; | ||
php.writeFile( | ||
`${docroot}/wp-content/plugins/test-plugin.php`, | ||
`<?php /**\n * Plugin Name: Test Plugin */` | ||
); | ||
await activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}); | ||
await expect( | ||
activatePlugin(php, { | ||
pluginPath: 'test-plugin.php', | ||
}) | ||
).resolves.toBeUndefined(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,5 @@ | ||
import { phpVar } from '@php-wasm/util'; | ||
import { StepHandler } from '.'; | ||
import { logger } from '@php-wasm/logger'; | ||
|
||
/** | ||
* @inheritDoc activatePlugin | ||
* @example | ||
|
@@ -39,18 +37,18 @@ export const activatePlugin: StepHandler<ActivatePluginStep> = async ( | |
progress?.tracker.setCaption(`Activating ${pluginName || pluginPath}`); | ||
|
||
const docroot = await playground.documentRoot; | ||
const result = await playground.run({ | ||
const activatePluginResult = await playground.run({ | ||
code: `<?php | ||
define( 'WP_ADMIN', true ); | ||
require_once( ${phpVar(docroot)}. "/wp-load.php" ); | ||
require_once( ${phpVar(docroot)}. "/wp-admin/includes/plugin.php" ); | ||
require_once( getenv('DOCROOT') . "/wp-load.php" ); | ||
require_once( getenv('DOCROOT') . "/wp-admin/includes/plugin.php" ); | ||
|
||
// Set current user to admin | ||
wp_set_current_user( get_users(array('role' => 'Administrator') )[0]->ID ); | ||
|
||
$plugin_path = ${phpVar(pluginPath)}; | ||
$plugin_path = getenv('PLUGIN_PATH'); | ||
$response = false; | ||
if (!is_dir($plugin_path)) { | ||
if ( ! is_dir( $plugin_path)) { | ||
$response = activate_plugin($plugin_path); | ||
} | ||
|
||
|
@@ -65,22 +63,103 @@ export const activatePlugin: StepHandler<ActivatePluginStep> = async ( | |
} | ||
} | ||
|
||
if ( null === $response ) { | ||
die('Plugin activated successfully'); | ||
} else if ( is_wp_error( $response ) ) { | ||
throw new Exception( $response->get_error_message() ); | ||
if ( is_wp_error($response) ) { | ||
die( $response->get_error_message() ); | ||
} else if ( false === $response ) { | ||
die( "The activatePlugin step wasn't able to find the plugin $plugin_path." ); | ||
} | ||
|
||
throw new Exception( 'Unable to activate plugin' ); | ||
`, | ||
env: { | ||
PLUGIN_PATH: pluginPath, | ||
DOCROOT: docroot, | ||
}, | ||
}); | ||
if (result.text !== 'Plugin activated successfully') { | ||
logger.debug(result); | ||
throw new Error( | ||
`Plugin ${pluginPath} could not be activated – WordPress exited with no error. ` + | ||
`Sometimes, when $_SERVER or site options are not configured correctly, ` + | ||
`WordPress exits early with a 301 redirect. ` + | ||
`Inspect the "debug" logs in the console for more details` | ||
if (activatePluginResult.text) { | ||
logger.warn( | ||
`Plugin ${pluginPath} activation printed the following bytes: ${activatePluginResult.text}` | ||
); | ||
} | ||
|
||
/** | ||
* Instead of checking the plugin activation response, | ||
* check if the plugin is active by looking at the active plugins list. | ||
* | ||
* We have to split the activation and the check into two PHP runs | ||
* because some plugins might redirect during activation, | ||
bgrgicak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* which would prevent any output that happens after activation from being returned. | ||
* | ||
* Relying on the plugin activation response is not reliable because if the plugin activation | ||
* produces any output, WordPress will assume it's an activation error. | ||
* See WordPress source code for more details: | ||
* https://github.com/WordPress/wordpress-develop/blob/6.7/src/wp-admin/includes/plugin.php#L733 | ||
* | ||
* Because some plugins can create an output, we need to use output buffering | ||
* to ensure the 'true' response is not polluted by other outputs. | ||
* If the plugin activation fails, we will return the buffered output as it might | ||
* contain more information about the failure. | ||
*/ | ||
const isActiveCheckResult = await playground.run({ | ||
code: `<?php | ||
ob_start(); | ||
require_once( getenv( 'DOCROOT' ) . "/wp-load.php" ); | ||
|
||
/** | ||
* Extracts the relative plugin path from either an absolute or relative plugin path. | ||
* | ||
* Absolute paths starting with plugin directory (e.g., '/wordpress/wp-content/plugins/test-plugin/index.php') | ||
* should be converted to relative paths (e.g., 'test-plugin/index.php') | ||
* | ||
* Directories should finish with a trailing slash to ensure we match the full plugin directory name. | ||
* | ||
* Examples: | ||
* - '/wordpress/wp-content/plugins/test-plugin/index.php' → 'test-plugin/index.php' | ||
* - '/wordpress/wp-content/plugins/test-plugin/' → 'test-plugin/' | ||
* - '/wordpress/wp-content/plugins/test-plugin' → 'test-plugin/' | ||
* - 'test-plugin/index.php' → 'test-plugin/index.php' | ||
* - 'test-plugin/' → 'test-plugin/' | ||
* - 'test-plugin' → 'test-plugin/' | ||
*/ | ||
$plugin_directory = WP_PLUGIN_DIR . '/'; | ||
$relative_plugin_path = getenv( 'PLUGIN_PATH' ); | ||
if (strpos($relative_plugin_path, $plugin_directory) === 0) { | ||
$relative_plugin_path = substr($relative_plugin_path, strlen($plugin_directory)); | ||
bgrgicak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if ( | ||
pathinfo($relative_plugin_path, PATHINFO_EXTENSION) !== 'php' && | ||
substr($relative_plugin_path, -1) !== '/' && | ||
is_dir( $plugin_directory . $relative_plugin_path ) | ||
) { | ||
$relative_plugin_path = $relative_plugin_path . '/'; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good call, I added it in dde935a. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, for brevity: if ( is_dir( $plugin_directory . $relative_plugin_path ) ) {
$relative_plugin_path = rtrim( $relative_plugin_path, '/' ) . '/';
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, this is much cleaner now. bb24b49 |
||
|
||
$active_plugins = get_option( 'active_plugins' ); | ||
foreach ( $active_plugins as $plugin ) { | ||
if ( substr( $plugin, 0, strlen( $relative_plugin_path ) ) === $relative_plugin_path ) { | ||
ob_end_clean(); | ||
bgrgicak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
die( 'true' ); | ||
} | ||
} | ||
die( ob_get_flush() ?: 'false' ); | ||
`, | ||
env: { | ||
DOCROOT: docroot, | ||
PLUGIN_PATH: pluginPath, | ||
}, | ||
}); | ||
|
||
if (isActiveCheckResult.text === 'true') { | ||
// Plugin activation was successful, yay! | ||
return; | ||
} | ||
bgrgicak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (isActiveCheckResult.text !== 'false') { | ||
logger.debug(isActiveCheckResult.text); | ||
} | ||
throw new Error( | ||
`Plugin ${pluginPath} could not be activated – WordPress exited with no error. ` + | ||
`Sometimes, when $_SERVER or site options are not configured correctly, ` + | ||
`WordPress exits early with a 301 redirect. ` + | ||
`Inspect the "debug" logs in the console for more details.` | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, this seems to contradict the other comment:
So is output an activation error, or is it okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is just incomplete. I explained now better, why a WP_Error during plugin activation doesn't mean the plugin isn't active. 0c09fc0 There is also a section about this in the PR description with a screenshot.