Skip to content

22: Redirect only where a standalone really did go away #36

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.

Sign up for GitHub

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

Open
nikolaystrikhar wants to merge 1 commit into
base: 21-packaging-hygiene
Choose a base branch
from
Open

22: Redirect only where a standalone really did go away #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions src/Conflict/Resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ public function resolve_all(): void {
return;
}

$deactivated = false;
// "A standalone is gone", not "a deactivation was attempted". The redirect below is only ever
// worth taking on the first of those, and taking it on the second is a loop.
$standalone_gone = false;

// The reader rather than a registrar of our own: it drains the registrations still buffered
// on the facade before it reads, and a registrar asked directly would miss anything
Expand All @@ -132,7 +134,7 @@ public function resolve_all(): void {
}

if ( $this->resolve( $sub_plugin ) ) {
$deactivated = true;
$standalone_gone = true;
}
} catch ( Throwable $thrown ) {
_doing_it_wrong(
Expand All @@ -151,7 +153,12 @@ public function resolve_all(): void {
// active, and an `exit` on the first would leave the second's standalone running with no
// notice raised about it — and would take the load pass at the next priority with it, so
// nothing bundled loaded on the request that was supposed to fix the conflict.
if ( $deactivated ) {
//
// And only where a standalone really did go away. A request that deactivated nothing has
// nothing to shed from memory, so re-requesting the screen would arrive at the same conflict
// — while the notices this pass queued are waiting on a request that reaches
// `all_admin_notices`, which a redirect never does.
if ( $standalone_gone ) {
$this->redirect();
}
}
Expand All @@ -163,7 +170,7 @@ public function resolve_all(): void {
*
* @throws Config_Exception When no hook prefix has been set, or a container binding is unusable.
*
* @return bool Whether the standalone was deactivated.
* @return bool Whether the standalone is gone -- not whether deactivating it was attempted.
*/
protected function resolve( Sub_Plugin $sub_plugin ): bool {
$policy = $sub_plugin->get_conflict_policy();
Expand All @@ -183,7 +190,17 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool {
case Conflict_Policy::DEACTIVATE:
$this->deactivate( $sub_plugin );

return true;
// Asked again rather than assumed, because turning the standalone off is not the same
// event as the standalone being off. A site or mu-plugin filtering
// `option_active_plugins` puts it straight back, a host may have rebound
// `Plugin\Contracts\Deactivator_Interface` to something that does nothing, and a
// rebound `Plugin\Contracts\Checker_Interface` may mean by "active" something
// `deactivate_plugins()` never touches. Answering true on a standalone that is still
// running redirects to the screen the user asked for, where the next request detects
// the same conflict and redirects again -- until the browser gives up and the whole of
// wp-admin is out of reach, with the merge notice never drawn because every one of
// those requests exits before `all_admin_notices`.
return ! $this->detector->is_in_conflict( $sub_plugin );

// NOTICE_ONLY, and anything is_valid() would accept that this switch has grown no
// branch for. The default sits on the branch that only talks, never on the one that
Expand Down
179 changes: 173 additions & 6 deletions tests/unit/Conflict/ResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,17 @@ class ResolverTest extends WPTestCase {
*/
private $deactivations = [];

/**
* Standalones the stubbed WordPress keeps reporting as active however often they are deactivated.
*
* The site that filters `option_active_plugins` to put a plugin back, the host that rebound the
* deactivator to a no-op, and the rebound checker that means something else by "active" all look
* like this from in here: `deactivate_plugins()` was called and the plugin is still running.
*
* @var string[]
*/
private $immovable_standalones = [];

/**
* @var string|null
*/
Expand All @@ -91,7 +102,8 @@ public function setUp(): void {
// uopz cannot stub a function that does not exist yet.
require_once ABSPATH . 'wp-admin/includes/plugin.php';

$this->deactivations = [];
$this->deactivations = [];
$this->immovable_standalones = [];

// The request the whole file describes: an admin GET of an ordinary screen. Both keys are set
// rather than inherited, because the redirect reads REQUEST_URI for the screen to re-request and
Expand Down Expand Up @@ -242,14 +254,23 @@ public function __construct() {
}

/**
* In conflict when first asked about a sub-plugin, and gone when asked again — which is
* what the bound deactivator standing between the two questions is meant to have
* accomplished. Frozen at true it would describe a standalone that survived, which is
* `test_a_standalone_that_survives_deactivation_does_not_redirect`'s subject and not this
* test's.
*
* @param Sub_Plugin $sub_plugin Sub-plugin to test.
*
* @return bool
*/
public function is_in_conflict( Sub_Plugin $sub_plugin ): bool {
$this->asked[] = $sub_plugin->get_slug();
$slug = $sub_plugin->get_slug();
$first_ask = ! in_array( $slug, $this->asked, true );

return true;
$this->asked[] = $slug;

return $first_ask;
}
};

Expand Down Expand Up @@ -306,7 +327,9 @@ static function () use ( $detector ): Detector {

$this->capture_resolution();

$this->assertSame( [ 'give-recurring' ], $detector->asked );
// Twice: once to find the conflict, and once after the deactivation to find out whether it is
// still there. The second ask is what decides the redirect.
$this->assertSame( [ 'give-recurring', 'give-recurring' ], $detector->asked );
$this->assertSame( [ 'give-recurring/give-recurring.php' ], $deactivator->deactivated );
$this->assertSame( [ 'give-recurring' ], $notices->merge_notices );
$this->assertSame(
Expand Down Expand Up @@ -477,6 +500,105 @@ public function test_it_deactivates_without_redirecting_once_the_headers_are_sen
);
}

/**
* The redirect exists to re-request the screen with the standalone's code out of memory, and a
* standalone that is still active has none of that to offer. Redirecting anyway is a loop: the next
* request detects the same conflict, deactivates to no effect, redirects again, and the browser
* gives up with the admin out of reach. The merge notice is what makes it silent — a request that
* exits never reaches `all_admin_notices`, so the one explanation the site owner would get is
* destroyed by the loop that made them need it.
*
* Nothing exotic is required to get here: a site or mu-plugin filtering `option_active_plugins`
* puts the standalone straight back into the active list, which is a pattern `learndash-core`
* itself uses.
*/
public function test_a_standalone_that_survives_deactivation_does_not_redirect(): void {
$this->standalone_is( true );
$this->standalone_survives_deactivation( 'give-recurring/give-recurring.php' );
$this->register();

$this->resolve_all();

$this->assertCount( 1, $this->deactivations, 'The policy still runs: deactivation is asked for.' );
$this->assertArrayHasKey(
'give-recurring:merge',
$this->queued_notices(),
'The request goes on rendering, so the notice explaining the deactivation must survive to be drawn.'
);
}

/**
* The same failure through the seam a host owns rather than through the site's own filters: a
* bound `Deactivator_Interface` that records and does nothing leaves the standalone exactly where
* it was, and the checker behind the detector says so without any help from this test.
*/
public function test_a_rebound_deactivator_that_turns_nothing_off_does_not_redirect(): void {
$deactivator = new class() implements Deactivator_Interface {
/**
* @var string[]
*/
public $deactivated = [];

/**
* @param string $basename Plugin basename.
*
* @return void
*/
public function deactivate( string $basename ): void {
$this->deactivated[] = $basename;
}
};

// Before the provider, which is where an interface seam goes: nothing can build an interface
// unprompted, so the provider reads the binding as the host's and leaves it alone.
$container = new Test_Container();
$container->singleton(
Deactivator_Interface::class,
static function () use ( $deactivator ): Deactivator_Interface {
return $deactivator;
}
);

$this->set_up_container( $container );

$this->standalone_is( true );
$this->register();

$this->resolve_all();

$this->assertSame( [ 'give-recurring/give-recurring.php' ], $deactivator->deactivated );
$this->assertSame(
[],
$this->deactivations,
'The bound deactivator is what deactivates, so nothing was ever taken out of the active list.'
);
$this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() );
}

/**
* One stubborn standalone must not cost the site the redirect the other one earned. The request
* still has a plugin's code in memory that a fresh one would shed, so it is still worth taking —
* and it cannot loop for ever on the strength of the first, because the request it lands on
* deactivates to no effect and stops there.
*/
public function test_it_still_redirects_when_one_of_two_standalones_really_went_away(): void {
$this->standalone_is( true );
$this->standalone_survives_deactivation( 'give-recurring/give-recurring.php' );
$this->register();
$this->register_fee_recovery();

$this->capture_resolution();

$this->assertSame(
[ 'give-recurring/give-recurring.php', 'give-fee-recovery/give-fee-recovery.php' ],
array_column( $this->deactivations, 'plugins' )
);

$queued = $this->queued_notices();
$this->assertArrayHasKey( 'give-recurring:merge', $queued );
$this->assertArrayHasKey( 'give-fee-recovery:merge', $queued );
}

public function test_deactivate_is_the_default_policy(): void {
$this->standalone_is( true );
$this->register();
Expand Down Expand Up @@ -847,11 +969,56 @@ private function register_fee_recovery( array $overrides = [] ): void {
* ORs the network check in itself, so stubbing is_plugin_active_for_network() alongside it
* would be inert and would read as though a network path were being exercised.
*
* @param bool $active Whether the standalone is active.
* Active *until it has been deactivated*, rather than active for ever. The resolver asks a second
* time before it redirects, so a stub frozen at true would describe a site where deactivation
* never works and every deactivating test would assert against that instead of the ordinary case.
* The recorded calls are what it reads, so the two stubs answer the same question consistently and
* a standalone deliberately made immovable stays that way.
*
* @param bool $active Whether the standalone starts out active.
*
* @return void
*/
private function standalone_is( bool $active ): void {
$this->setFunctionReturn( 'is_plugin_active', $active );
if ( ! $active ) {
$this->setFunctionReturn( 'is_plugin_active', false );

return;
}

// uopz runs a replacement with no class scope, so $this is fatal inside the closure. Both
// properties are bound by reference instead, which is also what lets a test arrange the
// immovable list after this call. See tests/README.md.
$deactivations = &$this->deactivations;
$immovable = &$this->immovable_standalones;

$this->setFunctionReturn(
'is_plugin_active',
static function ( $basename ) use ( &$deactivations, &$immovable ): bool {
if ( in_array( $basename, $immovable, true ) ) {
return true;
}

foreach ( $deactivations as $deactivation ) {
if ( in_array( $basename, (array) $deactivation['plugins'], true ) ) {
return false;
}
}

return true;
},
true
);
}

/**
* A standalone WordPress keeps reporting as active however often it is deactivated.
*
* @param string $basename Standalone plugin basename.
*
* @return void
*/
private function standalone_survives_deactivation( string $basename ): void {
$this->immovable_standalones[] = $basename;
}
}
29 changes: 24 additions & 5 deletions tests/unit/Scenario/HostTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ public function test_a_host_binding_reaches_every_step_of_the_request(): void {
/**
* Basenames this checker reports as active.
*
* Writable, because the standalone really does go away between the two requests below and a
* checker that never noticed would resolve the same conflict for ever.
* Writable, because the host's own deactivator is what empties it — the resolver asks this
* checker again after deactivating, and only redirects on an answer that changed.
*
* @var string[]
*/
Expand Down Expand Up @@ -114,16 +114,34 @@ public function is_active( string $basename ): bool {
*/
public $basenames = [];

/**
* What turning the plugin off does to the site this test describes.
*
* A double that only recorded would describe a site where deactivation never works, which
* is a different scenario — and one the resolver deliberately refuses to redirect on.
*
* @var callable|null
*/
public $turns_off = null;

/**
* @param string $basename Plugin basename.
*
* @return void
*/
public function deactivate( string $basename ): void {
$this->basenames[] = $basename;

if ( $this->turns_off !== null ) {
( $this->turns_off )( $basename );
}
}
};

$deactivator->turns_off = static function ( string $basename ) use ( $checker ): void {
$checker->active = array_values( array_diff( $checker->active, [ $basename ] ) );
};

$checker->active = [ self::STANDALONE ];

// Really active, so that the default deactivator would have emptied this option had it been
Expand Down Expand Up @@ -178,9 +196,10 @@ static function () use ( $activator ): Activator_Interface {
$this->assertSame( [ self::STANDALONE ], $deactivator->basenames, 'The host deactivator is the one asked to turn it off.' );
$this->assertSame( [ self::SLUG ], $writer->merge_notices, 'The host notice writer is told what happened.' );

// What the host's own deactivator did, as far as its own checker is concerned. Nothing is
// re-registered between the two — this is the next page view, not a second bootstrap.
$checker->active = [];
// The host's own deactivator has already emptied its own checker, which is what let the
// request above end in a redirect at all. Nothing is re-registered between the two — this is
// the next page view, not a second bootstrap.
$this->assertSame( [], $checker->active, 'The host deactivator turned the standalone off for the host checker.' );

$this->run_request();

Expand Down
Loading