[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066

bgrgicak · 2024-12-10T09:55:46Z

This PR prevents the activatePlugin step from failing if a plugin was successfully activated, by checking the active_plugins option instead of relying on the activate_plugin return value.

This PR splits the plugin activation from checking if the plugin is active into two scripts.
The activatePlugin step will be successful if a plugin is listed as active even if there is an error during activation.

This reflects how WordPress works when you enable a plugin in WP-admin. WP-admin will return an error notice if a plugin produces an error during activation, but if it can it will still activate the plugin (see screenshot below).

Plugin activation errors

Relying on the activate_plugin return value is not reliable because if the plugin activation produces any output, it will be threaded as an error. It's current from WordPress to assume that there was an error, but WordPress still might activate the plugin, so there is no need for the activatePlugin step to fail.

HTTP response status checks

Some plugins (for example wp-optimizer) will redirect the request after the plugin was activated to a different page.
This will result in a 301/302 HTTP response status instead of 200, so we can't use the HTTP response status 200 to validate if a plugin was successfully activated.

It also means that any code that was supposed to run after activate_plugin in our PHP plugin activation script won't run. This prevents us from checking if the plugin was activated in the same PHP script that activates a plugin. To work around this we now have a separate script that checks if the plugin is already activated.

Testing Instructions (or ideally a Blueprint)

CI

Manual testing

Open this Blueprint it will install wp-optimizer which will redirect to a different page during activation.

adamziel · 2024-12-10T17:47:14Z

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

+	 * a 200 status code response with the text 'Plugin activated successfully'
+	 * or a redirect status code (300-399) response with the text ''
+	 *
+	 * Some plugins redirect on activation.


How do we know it's not an error-related redirection? I wonder how wp-cli solves this. Not that we should load it just to active a plugin, but maybe there's some inspiration there.

WP-CLI checks is_wp_error https://github.com/wp-cli/extension-command/blob/main/src/Plugin_Command.php#L384

I updated my code to also cover scenarios when plugins create output. It contains a explanation why I think we shouldn't rely on the is_wp_error check https://github.com/WordPress/wordpress-playground/pull/2066/files#diff-4a02b123bb6311fff3ec089ca1ade75c75f1af35aaa4f1a717f14d444fc9b485R81-R84.

… a notice This PR ensures the autologin step won't cause a perpetual white screen of death when one of the plugins displays a notice. The autologin flow consists of a bunch of checks, setcookie() call, a header('Location') call, and an `exit;`. However, when the headers are already sent, the cookies won't be set, the redirect won't work, but the script will still. This, effectively, prevents loading any WordPress page. This PR adds a `headers_sent()` check to stop the autologin step early in those scenarios when we can't finish the login. ## Testing instructions * CI has a new E2E test – confirm everything is green * Try this Blueprint. It should open a white screen with errors. Now change the landing page to "/". It should open the homepage with a few errors at the top. Before this PR, you'd see a blank page instead of the homepage. ```json { "plugins": [ "pronamic-ideal" ], "preferredVersions": { "php": "8.2", "wp": "6.7" }, "features": { "networking": true }, "login": true, "landingPage": "/wp-admin/update-core.php", "steps": [ { "step": "defineWpConfigConsts", "consts": { "WP_DEBUG": true, "WP_DEBUG_DISPLAY": true } }, { "step": "setSiteLanguage", "language": "nl_NL" } ] } ```

…plugin-activation-fails-on-redirect

…gin logs a notice" This reverts commit 2f45f25.

zaerl

I find this a solid, more inclusive solution than the preexisting one. Good work! 🚢

adamziel · 2024-12-17T13:00:20Z

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

-				die('Plugin activated successfully');
-			} else if ( is_wp_error( $response ) ) {
-				throw new Exception( $response->get_error_message() );
+			if (is_wp_error($response)) {


What would it mean when $response is null or false and how should we act in each of these cases? Just ignore it and rely on the check below? Also, why replace throw with die()?

When using throw a WP_Error caused by the plugin activation would cause PHP to fail with a 255 response code and we wouldn't be able to read the error message AFAIK.

We would also need to use try/catch for the activation code which would be less elegant, especially in a case where we don't consider all WP_Errors created by the the activation process to be errors for us.

More details about why we don't consider all WP_Errors to be activation errors are documented here https://github.com/WordPress/wordpress-playground/pull/2066/files/aaf683d47f401d18f9958a8437b7abe4f428d915#diff-4a02b123bb6311fff3ec089ca1ade75c75f1af35aaa4f1a717f14d444fc9b485R81

What would it mean when $response is null or false and how should we act in each of these cases? Just ignore it and rely on the check below?

null is success, and I deliberately left the response empty. Otherwise it would be printed to logs (or we would need to account for it in a if) and it doesn't provide any value.

false means the plugin couldn't be found by the activatePlugin step. I added a check for it now in 8627934. The error message I added is is different from the error message provided by WordPress when a plugin isn't found so that we will be able to distinguish between these two.

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

adamziel · 2024-12-17T13:10:07Z

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

+			)}. "/wp-content/plugins/", '', ${phpVar(pluginPath)} );
+			$active_plugins = get_option( 'active_plugins' );
+			foreach ( $active_plugins as $plugin ) {
+				if ( strpos( $plugin, $relative_plugin_path ) === 0 ) {


This would yield a false positive if one plugin is called gutenberg and another one is called gutenberg-beta. Let's account for that.

How would this happen? We required absolute or relative plugin paths for plugin activation, not slugs.

gutenberg/gutenberg.php wouldn't result in true if gutenberg-beta/guternberg.php was active.

My suggestion for the whole section is to be more explicit on why you use str_replace vs substr or strpos vs ===. The former ones can be considered "fuzzy" while the latter ones are more precise. We should opt for the latter if possible and if not, explain why this is needed in a comment.

I have a working solution, but it's not clean. Let me think a bit more about it.

I update the is active check to either match the full PLUGIN_DIR/PLUGIN.php pattern or PLUGIN_DIR/.
If it's a PLUGIN_DIR/ the trailing slash will ensure we are checking the full plugin directory name and not a partial name.

Let me know what you think of the implementation https://github.com/WordPress/wordpress-playground/pull/2066/files#diff-4a02b123bb6311fff3ec089ca1ade75c75f1af35aaa4f1a717f14d444fc9b485R101-R147

This is looking really good, thank you @bgrgicak! I left a few minor nitpicks, no major changes needed.

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

adamziel

Good idea! I left a few nitpicks inline

adamziel · 2024-12-19T11:29:11Z

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

+			 * - 'test-plugin/' → 'test-plugin/'
+			 * - 'test-plugin' → 'test-plugin/'
+			 */
+			$plugin_directory = getenv( 'DOCROOT' ) . "/wp-content/plugins/";


This is fine today, in the future we'll need to rely on WP_CONTENT_DIR or WP_PLUGIN_DIR

Done 2efc8e2#diff-4a02b123bb6311fff3ec089ca1ade75c75f1af35aaa4f1a717f14d444fc9b485R122

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

adamziel · 2024-12-19T11:30:52Z

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

+				pathinfo($relative_plugin_path, PATHINFO_EXTENSION) !== 'php' &&
+				substr($relative_plugin_path, -1) !== '/'
+			) {
+				$relative_plugin_path = $relative_plugin_path . '/';


I'd add an is_dir() check, otherwise we're assuming something about a file structure based on an arbitrary string

I'd add an is_dir() check

Good call, I added it in dde935a.

Or, for brevity:

if ( is_dir( $plugin_directory . $relative_plugin_path ) ) { $relative_plugin_path = rtrim( $relative_plugin_path, '/' ) . '/'; }

Thanks, this is much cleaner now. bb24b49

packages/playground/blueprints/src/lib/steps/activate-plugin.ts

Co-authored-by: Adam Zieliński <adam@adamziel.com>

…g a slash

adamziel · 2024-12-19T13:05:48Z

packages/playground/blueprints/src/lib/steps/activate-plugin.spec.ts

+			activatePlugin(php, {
+				pluginPath: 'test-plugin.php',
+			})
+		).resolves.toBeUndefined();


The return values doesn't seem relevant here, what we're really after is resolves.not.toThrow(). Not a blocker since an exception would fail the test anyway

Done c0dd231

adamziel · 2024-12-19T13:07:08Z

packages/playground/blueprints/src/lib/steps/activate-plugin.spec.ts

+		).resolves.toBeUndefined();
+	});
+
+	it('should activate a plugin if it produces a output during activation', async () => {


Wait, this seems to contradict the other comment:

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

So is output an activation error, or is it okay?

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.

…in isn'a active

adamziel · 2024-12-19T20:04:27Z

Lovely, thank you @bgrgicak!

…ng activation or produces an output (#2066)

Prevent plugin activation error if plugin redirects during activation

6fed4da

bgrgicak self-assigned this Dec 10, 2024

adamziel reviewed Dec 10, 2024

View reviewed changes

bgrgicak and others added 2 commits December 13, 2024 08:14

Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect

2ca6335

bgrgicak changed the base branch from trunk to prevent-autologin-white-screen-of-death December 17, 2024 07:08

bgrgicak added 3 commits December 17, 2024 08:09

Merge branch 'prevent-autologin-white-screen-of-death' into fix/1979-…

3d69181

…plugin-activation-fails-on-redirect

Use options to validate if a plugin is active

d1ee3c9

Explain why we had to split into two runs

1747574

bgrgicak changed the base branch from prevent-autologin-white-screen-of-death to trunk December 17, 2024 08:07

bgrgicak added 5 commits December 17, 2024 09:08

Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect

75cd7b4

Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect

a46d412

Revert "[Blueprints] Prevent WSOD when autologin is enabled and a plu…

7204067

…gin logs a notice" This reverts commit 2f45f25.

Always return activation errors

7aa4329

Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect

aaf683d

bgrgicak marked this pull request as ready for review December 17, 2024 09:48

bgrgicak requested a review from adamziel December 17, 2024 09:48

zaerl self-requested a review December 17, 2024 10:38

zaerl approved these changes Dec 17, 2024

View reviewed changes

bgrgicak changed the title ~~[Blueprints] Prevent plugin activation error if plugin redirects during activation~~ [Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output Dec 17, 2024

adamziel reviewed Dec 17, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Show resolved Hide resolved

adamziel reviewed Dec 17, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed Dec 17, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed Dec 17, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed Dec 17, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed Dec 17, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

adamziel requested changes Dec 17, 2024

View reviewed changes

Merge branch 'trunk' into fix/1979-plugin-activation-fails-on-redirect

d255141

Add test for double plugin activation

2dd659e

bgrgicak requested a review from adamziel December 18, 2024 07:39

Ensure all supported plugin paths can be activated

9b148b2

bgrgicak marked this pull request as draft December 18, 2024 13:03

bgrgicak added 2 commits December 19, 2024 07:39

Clear up is active check

622099d

Add trailing slash activation test

d7e7827

bgrgicak marked this pull request as ready for review December 19, 2024 06:45

bgrgicak requested a review from akirk December 19, 2024 06:45

adamziel reviewed Dec 19, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Show resolved Hide resolved

adamziel reviewed Dec 19, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Show resolved Hide resolved

adamziel reviewed Dec 19, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Outdated Show resolved Hide resolved

adamziel reviewed Dec 19, 2024

View reviewed changes

packages/playground/blueprints/src/lib/steps/activate-plugin.ts Show resolved Hide resolved

bgrgicak and others added 3 commits December 19, 2024 12:58

Apply suggestions from code review

56cd875

Co-authored-by: Adam Zieliński <adam@adamziel.com>

Check if the plugin exists in the filesystem

2efc8e2

Check if the current relative plugin path is a directory before addin…

dde935a

…g a slash

bgrgicak requested a review from adamziel December 19, 2024 12:18

adamziel reviewed Dec 19, 2024

View reviewed changes

bgrgicak added 4 commits December 19, 2024 15:12

Use resolves.not.toThrow in tests

c0dd231

Explain why a WP_Error during plugin activation doesn't mean the plug…

0c09fc0

…in isn'a active

Remove .php check

e27c6fb

Use rtrim and only is_dir

bb24b49

bgrgicak requested a review from adamziel December 19, 2024 14:21

adamziel merged commit 406811f into trunk Dec 19, 2024
10 checks passed

adamziel deleted the fix/1979-plugin-activation-fails-on-redirect branch December 19, 2024 20:04

brandonpayton pushed a commit that referenced this pull request Dec 31, 2024

[Blueprints] Prevent plugin activation error if plugin redirects duri…

8faadbe

…ng activation or produces an output (#2066)

aaronjbaptiste mentioned this pull request Jan 13, 2025

Playground Fatal error when activating a plugin via github release #2126

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066

[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066

bgrgicak commented Dec 10, 2024 •

edited

Loading

adamziel Dec 10, 2024 •

edited

Loading

bgrgicak Dec 17, 2024

zaerl left a comment

adamziel Dec 17, 2024 •

edited

Loading

bgrgicak Dec 18, 2024

bgrgicak Dec 18, 2024

adamziel Dec 17, 2024

bgrgicak Dec 18, 2024

akirk Dec 18, 2024

bgrgicak Dec 18, 2024

bgrgicak Dec 19, 2024

adamziel Dec 19, 2024

adamziel left a comment

adamziel Dec 19, 2024

bgrgicak Dec 19, 2024

adamziel Dec 19, 2024

adamziel Dec 19, 2024 •

edited

Loading

bgrgicak Dec 19, 2024

adamziel Dec 19, 2024

bgrgicak Dec 19, 2024

adamziel Dec 19, 2024 •

edited

Loading

bgrgicak Dec 19, 2024

adamziel Dec 19, 2024

bgrgicak Dec 19, 2024

adamziel commented Dec 19, 2024

[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066

[Blueprints] Prevent plugin activation error if plugin redirects during activation or produces an output #2066

Conversation

bgrgicak commented Dec 10, 2024 • edited Loading

Plugin activation errors

HTTP response status checks

Testing Instructions (or ideally a Blueprint)

Manual testing

adamziel Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaerl left a comment

Choose a reason for hiding this comment

adamziel Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Dec 19, 2024

bgrgicak commented Dec 10, 2024 •

edited

Loading

adamziel Dec 10, 2024 •

edited

Loading

adamziel Dec 17, 2024 •

edited

Loading

adamziel Dec 19, 2024 •

edited

Loading

adamziel Dec 19, 2024 •

edited

Loading