Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#16335 closed defect (bug) (fixed)

Media Library search result sorting is broken

Reported by: stephdau Owned by:
Milestone: 3.1 Priority: normal
Severity: major Version: 3.1
Component: Administration Keywords: has-patch
Focuses: Cc:


Under the current trunk (r17344), when going to /wp-admin/upload.php and submitting a search, the latter search query gets lost when trying to change the results' sort order.

  1. go to /wp-admin/upload.php
  2. submit a search, like the always popular "img_"
  3. once you have results, try to sort
  4. watch how the results are now unfiltered (but sorted right).

The issue is with _POST. _GET (/wp-admin/upload.php?s=img_) works as expected and is carried throughout.

Found while testing r17344 merge on wp.com.

Attachments (1)

16335.patch (1.6 KB) - added by SergeyBiryukov 9 years ago.

Download all attachments as: .zip

Change History (16)

#1 @scribu
9 years ago

  • Milestone changed from Awaiting Review to 3.1

#2 @SergeyBiryukov
9 years ago

It's not just Media Library. Comments, Links and Users too.

#3 @scribu
9 years ago

  • Severity changed from normal to major

Ah, of course it doesn't work. The link that you click to sort a column isn't updated to contain the search string.

#4 @nacin
9 years ago

Need to revert all form method=POST back to GET again to fix this.

#5 @SergeyBiryukov
9 years ago

  • Keywords has-patch added; search media removed

#6 @nacin
9 years ago

Might as well just re-open that bulk action URI ticket now.

#7 @scribu
9 years ago

Patch works for me.

#8 @SergeyBiryukov
9 years ago

Plugins search form still has method="post", however there are no sortable columns now, so it's good.

#9 @ryan
9 years ago

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

(In [17345]) From post back to get. Props SergeyBiryukov. fixes #16335

#10 @scribu
9 years ago

Related: #16341

#11 @stephdau
9 years ago

Sweet! Thanks scribu, SergeyBiryukov, nacin and ryan.

#12 @_ck_
9 years ago

Did you seriously switch admin functions that can potentially pass up to several thousand bytes of data to GET via URL just avoid fixing the sort routine with POST?

That is just dumbfounding. It defies all "best practice" programming techniques not to mention the security issues it can create.

#13 @scribu
9 years ago

Thanks for the helpful comment, _ck_. You're sure helping the project along.

If it's so awful, just fork it and make it conform to whatever best practices you want. Show us mortals how it's done.

#14 @_ck_
9 years ago

scribu do you feel it's appropriate to attack me personally in trac when I am commenting on the approach and not any individual? If you feel this change was appropriate then defend the change, don't attack me.

I don't claim to be an expert but that's my point - anyone with a base level of experience should have seen this is a bad move.

Don't take my "opinion" on it, go ask on Stack Overflow for some independent feedback on the wisdom of changing a form (in a legacy environment no less) that passes up to several Kbytes of data from POST to GET.

Let's forget about the security implications for a moment.

There are browsers still used today that won't accept urls over 2000 bytes.

Some proxies won't accept urls over 255 characters.

#15 @scribu
9 years ago

You don't attack an individual, you just tacitly imply that we're all idiots. Of course we know that using POST would be better.

In fact, we did switch to POST, but then found bugs that couldn't be fixed in time for 3.1, so the decision was made to revert the change.

Note: See TracTickets for help on using tickets.