WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 11 days ago

#17552 assigned defect (bug)

Plugin editor incorrectly calls some files inactive.

Reported by: dd32 Owned by: DrewAPicture
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.1
Component: Plugins Keywords: has-patch needs-testing
Focuses: Cc:

Description

The plugin editor has a helpful bit of text at the top to specify if the current file is active on the site or not; This works fine in most cases, however, I've noticed that it only works for the plugin file itself.

Ie. Editing akismet/akismet.php will specify it's active, Editing akismet/admin.php will show as inactive.

This is due to the use of is_plugin_active() from memory. The solution to this would be to run is_plugin_active() on $_GET['plugin'] instead of the file being edited.

$_GET['plugin'] has it's own bug however, It's set to whichever file was edited before you loaded the current file (when you switch between files in a plugin that is), which in some cases will be correct, in many others when you're editing multiple files, It'll be incorrect.

Attachments (10)

17552.diff (530 bytes) - added by solarissmoke 5 years ago.
17552.patch (7.4 KB) - added by dd32 5 years ago.
17552-2.patch (7.2 KB) - added by WraithKenny 3 years ago.
refresh
17552.2.patch (7.4 KB) - added by DrewAPicture 12 months ago.
Refresh
17552.3.patch (8.0 KB) - added by DrewAPicture 12 months ago.
Query args sanity + self_admin_url()
17552.4.patch (7.9 KB) - added by DrewAPicture 12 months ago.
17552.5.patch (8.0 KB) - added by MattyRob 6 weeks ago.
17552.5b.patch (7.9 KB) - added by MattyRob 6 weeks ago.
17552.6.patch (8.2 KB) - added by MattyRob 6 weeks ago.
35788.diff (5.1 KB) - added by swissspidy 11 days ago.

Download all attachments as: .zip

Change History (31)

#1 @dd32
5 years ago

  • Summary changed from Plugin editor incorrectly calls some inactive. to Plugin editor incorrectly calls some files inactive.

@solarissmoke
5 years ago

#2 follow-ups: @solarissmoke
5 years ago

  • Keywords has-patch added; needs-patch removed

Seems to have been caused by a typo - the $plugin variable was being assigned to $_REQUEST['file'] instead of $_REQUEST['plugin'].

@dd32 I couldn't reproduce/understand the issue with $_GET['plugin']... With the patch I just posted, things seem to work fine?

#3 in reply to: ↑ 2 @dd32
5 years ago

Replying to solarissmoke:

Seems to have been caused by a typo - the $plugin variable was being assigned to $_REQUEST['file'] instead of $_REQUEST['plugin'].

Oddly enough, As much as it looks like a typo, it's not just a typo there, $file is referenced to as both the file being currently edited, and as the plugin being edited.. it looks like it was to fix the problem of $file not containing $plugin in many places (If you don't follow that, don't worry..)

@dd32 I couldn't reproduce/understand the issue with $_GET['plugin']... With the patch I just posted, things seem to work fine?

Sorry i didn't get back to you at the time, but yeah, I no longer see that problem after patching the editor code, started off with your patch and it grew as i noticed issues.. but never noticed that problem..

Work in progress patch showing what I've come up against attached

@dd32
5 years ago

#4 @WraithKenny
5 years ago

  • Cc Ken@… added

#5 in reply to: ↑ 2 @SergeyBiryukov
3 years ago

Replying to solarissmoke:

Seems to have been caused by a typo - the $plugin variable was being assigned to $_REQUEST['file'] instead of $_REQUEST['plugin'].

Related: [11500]

#7 @WraithKenny
3 years ago

dd32's patch seems to make a lot of sense.

the important bit is

if ( empty($plugin) ) {
    if ( ! empty($file) ) {
        $plugin = $file;
    } else {
        $plugin = array_keys($plugins);
        $plugin = $plugin[0];
    }
}

@WraithKenny
3 years ago

refresh

#8 @jayarjo
2 years ago

Why it ended up as:

if ( $file ) {
	$plugin = $file;
} elseif ( empty( $plugin ) ) {
	$plugin = array_keys($plugins);
	$plugin = $plugin[0];
}

The problem still there (WP 3.9.1). I think the reason behind this was not a typo, but an actual intention to list only sibling files to the one being edited. But no matter the reason, it feels like a wrong way to do it anyway. And it breaks the plugin selector above.

Last edited 2 years ago by jayarjo (previous) (diff)

#9 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 4.1

17552-2.patch appears to fix both this ticket and #24122.

Need to resolve the @todo's left there.

#10 @dd32
22 months ago

  • Milestone changed from 4.1 to Future Release

Close enough to release that we should circle back on this next cycle.

@DrewAPicture
12 months ago

Refresh

@DrewAPicture
12 months ago

Query args sanity + self_admin_url()

#11 @DrewAPicture
12 months ago

  • Milestone changed from Future Release to 4.4

17552.3.patch Refreshes the patch and introduces some sanity to the redirects with add_query_arg() and switches them to use self_admin_url() to cover single vs network admin bases.

Still need to cover the @TODO comments.

#12 @DrewAPicture
12 months ago

  • Keywords needs-testing added
  • Owner set to DrewAPicture
  • Status changed from new to assigned

17552.4.patch addresses the TODO items. In testing, I was getting some unexpected behavior with editing network-active plugins: 'phperror' is getting set every time with the patch applied. Gotta figure that out.

Everything else looks and works as expected. And this patch does, as mentioned, also fix #24122.

#13 @DrewAPicture
11 months ago

  • Milestone changed from 4.4 to Future Release

Still having trouble with the chicken and egg problem of the phperror flag not triggering properly. Let's pick this up in 4.5, punting.

#14 @SergeyBiryukov
6 weeks ago

#37664 was marked as a duplicate.

#15 @MattyRob
6 weeks ago

  • Keywords dev-feedback 2nd-opinion added

The first patch on new ticket 37664 seems to fix this issue with very little in terms of code changes and as far as I can tell works without breaking anything else. Worth a review?

#16 @MattyRob
6 weeks ago

Okay, on further testing the patch from 37664 is flawed, for example links from the plugins.php to the editor no longer work and there are error messages when attempting to edit plugins.

Attached is reworked patch from this ticket - testing it revealed that live updates to plugins in non-multisite installs result in plugin deactivating but not reactivation. That's fixed but probably needs more testing on multisite.

@MattyRob
6 weeks ago

@MattyRob
6 weeks ago

#17 @MattyRob
6 weeks ago

Please see the 'b' patch - some debug garbage removed from that patch while I was checking out the wp_redirect() calls.

@MattyRob
6 weeks ago

#18 @MattyRob
6 weeks ago

New patch fixes an issue where the 'Update File' button displayed as 'Update File and Attempt to Reactivate' when the plugin was actually still active. Was due to passing of 'phperror' in the URL incorrectly.

@swissspidy
11 days ago

#19 @swissspidy
11 days ago

  • Keywords needs-testing removed

35788.diff is an updated patch that solves this issue as well as #24122.

See #35788 for details.

#20 @swissspidy
11 days ago

  • Keywords needs-testing added; dev-feedback 2nd-opinion removed
  • Milestone changed from Future Release to 4.7

#21 @MattyRob
11 days ago

+1 for this in 4.7, I've been patching my WordPress sites with this patch and it's works as expected - I haven't found any remaining issues.

Note: See TracTickets for help on using tickets.