Make WordPress Core

Opened 3 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#60471 closed defect (bug) (fixed)

Plugin dependencies: Dependent of an inactive plugin can be deleted via bulk actions

Reported by: johnbillion's profile johnbillion Owned by: costdev's profile costdev
Milestone: 6.5 Priority: low
Severity: minor Version: trunk
Component: Plugins Keywords: has-patch needs-testing
Focuses: Cc:

Description

With plugin-b that depends on plugin-a via its Requires Plugins: header and neither plugin active on the site, it should not be possible to delete plugin-a, however this can be done via the Bulk Actions menu.

To reproduce:

  • Have plugin-b that depends on plugin-a
  • Neither plugin is active
  • Observe the message on the Plugins admin screen for plugin-a: Note: this plugin cannot be deactivated or deleted until the plugins that require it are deactivated or deleted
  • Check the checkbox next to plugin-a
  • In the Bulk Actions dropdown select Delete
  • Press Apply
  • Observe that plugin-a gets deleted

Change History (15)

#1 @johnbillion
3 weeks ago

  • Priority changed from normal to low
  • Severity changed from normal to minor

#2 @afragen
3 weeks ago

This is actually functioning as designed. The error is in the explainer text showing up when it shouldn’t.

#3 in reply to: ↑ description @knutsp
3 weeks ago

Replying to johnbillion:

With plugin-b that depends on plugin-a via its Requires Plugins: header and neither plugin active on the site, it should not be possible to delete plugin-a, however this can be done via the Bulk Actions menu.

No need to disallow deleting (an inactive) plugin no matter how many other (inactive) plugins that (potentially) depend on it. Dependency means to disallow activating depending plugins when depends on plugin is not active (inactive or missing), and disallow deactivating a plugin that other active plugin depends on.

Replying to afragen:

The error is in the explainer text showing up when it shouldn’t.

Yes, but why does this text mention deleting at all? Active plugins cannot be deleted through WordPress Admin. When not able to deactivate, not being able to delete is implicit.

I look forward to see how gracefully WordPress will handle the situation where a plugin that other active plugins depends on is suddenly deleted externally (from the file system), or deactivated by foreign manipulation of the active plugins option.

Last edited 3 weeks ago by knutsp (previous) (diff)

#4 @johnbillion
3 weeks ago

Clarification: I don't think the Plugin Dependencies feature should be blocking the deletion of inactive dependent plugins, but whatever it does it needs to be consistent.

This ticket was mentioned in PR #6073 on WordPress/wordpress-develop by @costdev.


3 weeks ago
#5

  • Keywords has-patch added

The intended behaviour of Plugin Dependencies is that dependencies cannot be deleted until their dependents are deleted.

While the 'Delete' link was removed from the plugin row, the bulk actions checkbox remained enabled, allowing for deletion of dependencies through bulk actions.

This was unintended behaviour and the bulk actions checkbox is now disabled as intended.

### Before
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/79332690/6367e6b6-4e57-48f6-8be6-c88baa2c62a3

### After
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/79332690/ff161083-2a32-411f-a305-b4dc33a98d4c

#6 @costdev
3 weeks ago

  • Keywords needs-testing added

Thanks for opening this ticket @johnbillion!

Reading the ticket and @afragen's response, in this case the bug is indeed that the bulk actions checkbox is enabled.

The feature as it currently stands prevents the deletion of a dependency until its dependents are deleted. That is, Addon for Foo must be deleted before Foo can be deleted.

I'd like to separate the handling of this bug from the determination about whether this aspect of the feature stays in Core. A decision on that is likely to be part of an upcoming Core discussion on the feature, and I think it may be best to have the intended behaviour working as expected so that the discussion has accurate data to consider.

PR 6073 should ensure - for now at least - the intended behaviour. Please test.

Last edited 3 weeks ago by costdev (previous) (diff)

#7 @costdev
3 weeks ago

  • Milestone changed from Awaiting Review to 6.5

#8 @costdev
3 weeks ago

  • Component changed from Upgrade/Install to Plugins

Nudging this into the Plugins component as it deals more with their display and handling once installed than the Upgrade/Install aspect.

#9 @huzaifaalmesbah
2 weeks ago

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/6073

Environment

  • WordPress: 6.5-alpha-56966-src
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 121.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Four 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.0.1

Screenshots

Before Apply Patch After Apply Patch ✅
https://i.ibb.co/HnF21Dm/Screenshot-2024-02-10-at-12-32-07-AM.png https://i.ibb.co/5KmVQhz/Screenshot-2024-02-10-at-12-32-47-AM.png

@huzaifaalmesbah commented on PR #6073:


2 weeks ago
#10

I tested this patch it's works perfectly. Thanks.

@afragen commented on PR #6073:


2 weeks ago
#11

Tested, works. ✅

#12 @costdev
2 weeks ago

  • Owner set to costdev
  • Status changed from new to assigned

@mukesh27 commented on PR #6073:


2 weeks ago
#13

Nice work ✅

#14 @costdev
2 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57620:

Plugins: Disable bulk actions for dependencies.

The intended behaviour of Plugin Dependencies is that dependencies cannot be deleted until their dependents are deleted.

While the 'Delete' link was removed from the plugin row, the bulk actions checkbox remained enabled, allowing for deletion of dependencies through bulk actions.

This was unintended behaviour and the bulk actions checkbox is now disabled as intended.

Follow-up to [57545].

Props johnbillion, afragen, knutsp, huzaifaalmesbah, mukesh27, costdev.
Fixes #60471.

@costdev commented on PR #6073:


2 weeks ago
#15

Committed in 57620.

Note: See TracTickets for help on using tickets.