Skip to content
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: plugins are activated with is_admin() false #523

Closed
akirk opened this issue Jun 5, 2023 · 6 comments
Closed

Blueprints: plugins are activated with is_admin() false #523

akirk opened this issue Jun 5, 2023 · 6 comments
Labels
Breaking change [Package][@wp-playground] Blueprints [Type] Bug An existing feature does not function as intended

Comments

@akirk
Copy link
Member

akirk commented Jun 5, 2023

What I specifically observe: The GlotPress plugin no longer properly initializes and fails to create its database tables. Specifically this section no longer runs:

if ( is_admin() && GP_DB_VERSION > get_option( 'gp_db_version' ) ) {
	require_once ABSPATH . 'wp-admin/includes/upgrade.php';
	require_once GP_PATH . GP_INC . 'install-upgrade.php';
	require_once GP_PATH . GP_INC . 'schema.php';
	gp_upgrade_db();
}

This code should run upon every plugin load but it is never executed and I suspect this is becauseis_admin() is false when the plugin is activated.

@adamziel
Copy link
Collaborator

adamziel commented Jun 5, 2023

Hey @akirk 👋

Sorry about that - ideally there won’t be breaking changes out of the blue.

Plugin activation was recently refactored from clicking the „activate” link as an admin to calling the activate_plugin() WordPress function.

I suspect that same change removed the autologin, which is fine on its own, but I would have preferred to catch it early and communicate accordingly.

Thinking about the future, Blueprints could really use a high test coverage - let’s expedite #463 to make it possible.

@adamziel adamziel added [Package][@wp-playground] Blueprints Breaking change [Type] Bug An existing feature does not function as intended labels Jun 5, 2023
@adamziel
Copy link
Collaborator

adamziel commented Jun 5, 2023

To fix, let’s make sure admin is the active user before calling activate_plugin() and activate_theme(). AFAIR adding set_current_user(0) to the blueprint handler would do it

@akirk
Copy link
Member Author

akirk commented Jun 5, 2023

Actually is_admin() refers to being inside wp-admin. So looking at the is_admin() code, you'd just need to define( 'WP_ADMIN', true )

@adamziel
Copy link
Collaborator

adamziel commented Jun 5, 2023

Actually I was wrong, is_admin() refers to „are we in wp-admin right now?”:

function is_admin() {
	if ( isset( $GLOBALS['current_screen'] ) ) {
		return $GLOBALS['current_screen']->in_admin();
	} elseif ( defined( 'WP_ADMIN' ) ) {
		return WP_ADMIN;
	}

	return false;
}

in this case, define( 'WP_ADMIN', true ) in the blueprint handler should fix it

@eliot-akira
Copy link
Collaborator

eliot-akira commented Jun 5, 2023

I suspect that same change removed the autologin

Looking back on the previous implementation of the blueprint step installPlugin, there was no auto-login, but it assumed that an admin user was logged in already.

The plugin install and activation were performed by requesting ("visiting") pages under /wp-admin with the current user when the blueprint step was called.

const pluginForm = await playground.request({
	url: '/wp-admin/plugin-install.php?tab=upload',
});

WIth the refactor, the step no longer goes through admin screens but invokes PHP and manually loads a minimal WordPress environment.

const requiredFiles = [
	`${await playground.documentRoot}/wp-load.php`,
	`${await playground.documentRoot}/wp-admin/includes/plugin.php`,
];
...
const result = await playground.run({
	code: `<?php
${requiredFiles.map((file) => `require_once( '${file}' );`).join('\n')}
$plugin_path = '${pluginPath}';
if (!is_dir($plugin_path)) {
	activate_plugin($plugin_path);
	return;
}
...

A backward-compatible solution would be to ensure that the current user is available before calling activate_plugin(). I'm not sure why that's not happening with wp-load.php.. Oh, I misunderstood. The function is_admin() has nothing to do with current user, it's about whether the page request is within /wp-admin.

is_admin() is false when the plugin is activated

I see, that's solved by PR #524 setting WP_ADMIN to true. (I wonder what side effects there could be by setting that constant.)


This situation seems possibly relevant to other blueprint steps, many of which are about "admin" actions. They might need to be performed with is_admin() as true.

The installAsset step is OK because it's only pure PHP, does not use any WP functions.

TheimportFile step is currently done by requesting pages within /wp-admin and parsing the result into DOM (here). That means, if we plan to rewrite it to be "universal" (not dependent on browser DOM), then the new implementation might need to define WP_ADMIN as true while calling the importer functions directly. Maybe not, though - it depends on the assumptions made by WP core and plugins, when is_admin() is expected to be true.

@akirk
Copy link
Member Author

akirk commented Jun 9, 2023

Thanks for the analysis! As this issue around is_admin() is fixed with #524, I'd opt to close this one and we can create a new issue if the assumption of a logged in user creates problems with other plugins?

@akirk akirk closed this as completed Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change [Package][@wp-playground] Blueprints [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants