WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#16549 closed defect (bug) (fixed)

Plugin searches maintains activated/deactivated URL parameters

Reported by: kawauso Owned by: ryan
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch commit 3.3-early
Focuses: Cc:

Description

Steps to reproduce:

  • Activate a plugin
  • Search for anything
  • A status message for the activation status will still appear

Attachments (6)

16549.diff (422 bytes) - added by solarissmoke 5 years ago.
16549.2.diff (435 bytes) - added by garyc40 5 years ago.
remove status query var when search plugins
16549.3.diff (394 bytes) - added by garyc40 5 years ago.
using $_SERVER['REQUEST_URI'] makes more sense
16549.4.diff (549 bytes) - added by garyc40 5 years ago.
restore 3.0.x behavior
16549.5.diff (438 bytes) - added by solarissmoke 5 years ago.
Make search form point to clean plugins.php
16549.4.2.diff (627 bytes) - added by kawauso 4 years ago.
Refresh of garyc40's patch

Download all attachments as: .zip

Change History (28)

comment:1 @SergeyBiryukov5 years ago

Same after deleting a plugin.

@solarissmoke5 years ago

comment:2 @solarissmoke5 years ago

  • Keywords has-patch added

comment:3 @dd325 years ago

Will that patch preserve any filters applied? (for example, Inactive plugins only?)

If not, we can utilise $_SERVER['REQUEST_URI'] somehow as that'll have any action-based queryvars striped from it already.

@garyc405 years ago

remove status query var when search plugins

@garyc405 years ago

using $_SERVER['REQUEST_URI'] makes more sense

comment:4 follow-up: @solarissmoke5 years ago

@dd32 no, but these filters were not preserved even before. Search results always show both inactive/active plugins.

comment:5 in reply to: ↑ 4 @garyc405 years ago

Replying to solarissmoke:

@dd32 no, but these filters were not preserved even before. Search results always show both inactive/active plugins.

This is unfortunately true. In 3.0.x, filters are not preserved.

I personally think filters should be preserved.

@garyc405 years ago

restore 3.0.x behavior

comment:6 follow-up: @dd325 years ago

I personally think filters should be preserved.

Filters are currently preserved for the next request, It's just that the Search does not abide by the filters.

Replace esc_attr() with esc_url() and 16549.3.diff should be right to go, Needs another devs thoughts however

comment:7 in reply to: ↑ 6 ; follow-up: @garyc405 years ago

Replying to dd32:

I personally think filters should be preserved.

Filters are currently preserved for the next request, It's just that the Search does not abide by the filters.

That's what I meant. In 3.0.x, the filters are not preserved when you search. So not preserving filters when searching is not a regression, and fixing it would be an enhancement (which probably means a separate ticket).

solarissmoke's patch will redirect a search executed in wp-admin/network/plugins.php to wp-admin/plugins.php . Plus there might be unexpected consequences when performing bulk action (paging being reset to 1 for example).

If this issue described in the ticket is considered a regression (which I think it is, albeit a minor one), restoring the search behavior in 3.0.x is probably the safest route to take. See 16549.4.diff .

comment:8 follow-up: @dd325 years ago

16549.4.diff

Doesnt work, Has exactly the same problems as current, except it doesnt include the filters in the url, You'd have to include REQUEST_URI in order to fix the actual issue here.

comment:9 in reply to: ↑ 8 @garyc405 years ago

Replying to dd32:

16549.4.diff

Doesnt work, Has exactly the same problems as current, except it doesnt include the filters in the url, You'd have to include REQUEST_URI in order to fix the actual issue here.

I tested it and the problem described in the ticket is gone after applying 16549.4.diff. Because method="get", all the current filters as well as status vars are cleared out when searching.

comment:10 @garyc405 years ago

To make it perfectly clear for reviewers, 16549.4.diff attempts to fix the bug outlined in the ticket description (and restore the behavior to that of 3.0.x), rather than attempting to preserve the filters when searching as well (which I think should be a separate enhancement ticket for 3.2).

If you want to restore the filters when searching, as dd32 said, 16549.3.diff is good to go with a small modification.

comment:11 follow-ups: @dd325 years ago

To make it perfectly clear for reviewers, 16549.4.diff attempts to fix the bug outlined in the ticket description (and restore the behavior to that of 3.0.x), rather than attempting to preserve the filters when searching as well (which I think should be a separate enhancement ticket for 3.2).

And like I said, It doesn't actually fix anything. It uses the same style as current in core(<form method="get" action="">) which results in the activation flags present in the current url being carried between requests.

I don't care about keeping the filters, but since it doesn't work now, it doesn't matter that it doesn't work afterwards, that can be left for 3.2 to decide what to do there.

I just want a minimal change which fixes the current bug :)

comment:12 in reply to: ↑ 11 @garyc405 years ago

Replying to dd32:

And like I said, It doesn't actually fix anything. It uses the same style as current in core(<form method="get" action="">) which results in the activation flags present in the current url being carried between requests.

Hmm, that's strange, because it works for me. I tested the patch several times since your previous comments and it seems to fix the problem.

Changing method to get on the search form doesn't pass along activation flags, in my case, at least on Chrome and Firefox.

Could you upload a screenshot somewhere?

comment:13 in reply to: ↑ 11 @garyc405 years ago

Replying to dd32:

It uses the same style as current in core(<form method="get" action="">)

In current trunk, the search box is inside <form method="post" action="">, which is the form used for bulk actions. What my patch does is separating the search box into a method="get" form, which fixes the issue. But still, it works for me, I'm not sure why it doesn't work for you.

comment:14 @dd325 years ago

Sorry about that, I'm here to eat my hat :)

No idea what happened there, but I tested that 3 damn times, and it didn't work.. Just re-applied the patch and it did work..

Either style is fine by then, .3 will probably be best off for 3.2 (due to ajax, and everything in the single form), .4 solves it to 3.0 compat.

comment:15 @dd325 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 3.1

Bumping to 3.1 for another dev to check

@solarissmoke5 years ago

Make search form point to clean plugins.php

comment:16 in reply to: ↑ 7 ; follow-up: @solarissmoke5 years ago

Replying to garyc40:

solarissmoke's patch will redirect a search executed in wp-admin/network/plugins.php to wp-admin/plugins.php . Plus there might be unexpected consequences when performing bulk action (paging being reset to 1 for example).

attachment:16549.5.diff fixes this.

So all of .3, .4 and .5 are working patches. IMHO .3 isn't very future-proof. Between the other two - devs can decide.

comment:17 in reply to: ↑ 16 @garyc405 years ago

Replying to solarissmoke:

attachment:16549.5.diff fixes this.

Anything you modify in the method="post" form affects bulk actions, so some more testing needs to be done with 16549.5.diff.

comment:18 @scribu5 years ago

16549.4.diff looks like the way to go, since we don't have sorting involved.

comment:19 @ryan5 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

Per bug scrub, punting to 3.2.

comment:20 @kawauso4 years ago

  • Keywords 3.3-early added; 3.2-early removed

Per chat with westi

@kawauso4 years ago

Refresh of garyc40's patch

comment:21 @kawauso4 years ago

  • Milestone changed from Future Release to 3.3

comment:22 @ryan4 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [18991]:

Use get method for plugin search form. Avoid lingering notices. Props garyc40. fixes #16549

Note: See TracTickets for help on using tickets.