WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 4 months ago

#17552 assigned defect (bug)

Plugin editor incorrectly calls some files inactive.

Reported by: dd32 Owned by: DrewAPicture
Milestone: Future Release 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 (6)

17552.diff (530 bytes) - added by solarissmoke 5 years ago.
17552.patch (7.4 KB) - added by dd32 4 years ago.
17552-2.patch (7.2 KB) - added by WraithKenny 3 years ago.
refresh
17552.2.patch (7.4 KB) - added by DrewAPicture 4 months ago.
Refresh
17552.3.patch (8.0 KB) - added by DrewAPicture 4 months ago.
Query args sanity + self_admin_url()
17552.4.patch (7.9 KB) - added by DrewAPicture 4 months ago.

Download all attachments as: .zip

Change History (19)

#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
4 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
4 years ago

#4 @WraithKenny
4 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
20 months 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 20 months ago by jayarjo (previous) (diff)

#9 @SergeyBiryukov
16 months 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
14 months ago

  • Milestone changed from 4.1 to Future Release

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

@DrewAPicture
4 months ago

Refresh

@DrewAPicture
4 months ago

Query args sanity + self_admin_url()

#11 @DrewAPicture
4 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
4 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
4 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.

Note: See TracTickets for help on using tickets.