WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#14927 closed defect (bug) (fixed)

Impossible to search for Images w/o Javascript

Reported by: hakre Owned by: westi
Milestone: 3.1 Priority: normal
Severity: critical Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

When Javascript is switched off (in trunk, or javascript on in 3.0.x) it's not possible to search images _only_ in Media.

It will search all media, not images only (as with javascript on on trunk, it will search only the current selection).

It's either a bug that it does not w/o javascript or it's a but that it does with javascript (as the button says "search media" and not "search images").

Browse /wp-admin/upload.php?post_mime_type=image and use the search form there to reproduce.

Attachments (6)

Ticket #14927.patch (1.2 KB) - added by paulwillen 3 years ago.
Please forgive me if this is not correct...... My 1st patch ever
fix-no-js-please.diff (634 bytes) - added by westi 3 years ago.
fix-no-js-please-everywhere.diff (1.6 KB) - added by westi 3 years ago.
fix-no-js-please-everywhere-use-get.14927.diff (1.8 KB) - added by filosofo 3 years ago.
fix-search-query-display-no-js.diff (504 bytes) - added by westi 3 years ago.
Fix search box population
14927.show.search.results.text.patch (585 bytes) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 hakre4 years ago

Scribu implemented mime-type filtered search for Ajax requests. See #14579.

comment:2 voyagerfan57614 years ago

  • Cc WordPress@… added

comment:3 scribu4 years ago

  • Owner set to scribu
  • Status changed from new to reviewing

comment:4 jane3 years ago

I think the bug is the label. Expected behavior is to search media whether javascript is there or not. We don't have an advanced search in core yet, so if it half-snuck in at some point, the interaction model for it was never officially worked out.

comment:5 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Severity changed from normal to critical

Searching without JavaScript is broken in general. Doesn't seem to have to do with the mime type.

These work: posts, pages, (ms) users, (ms) themes, (ms) plugins, sites.

These do not: media, comments, links.

paulwillen3 years ago

Please forgive me if this is not correct...... My 1st patch ever

comment:6 paulwillen3 years ago

  • Keywords has-patch added

comment:7 scribu3 years ago

  • Keywords commit added

Best solution, given the circumstances.

Ideally, the search input should have a separate <form> tag, but that would require duplicating a lot of hidden inputs.

comment:8 nacin3 years ago

Does that affect the bulk action limits with regards to URI length? I believe that was a primary reason for moving to post.

comment:9 scribu3 years ago

Yes, it does, but the move to 'post' was done in 3.1, so it's not a regression.

comment:10 nacin3 years ago

Why does this work? I imagine we have a GET check somewhere that should be REQUEST instead? We should fix the real bug here.

comment:11 scribu3 years ago

It doesn't work because $_REQUEST turns up empty for some reason.

comment:12 westi3 years ago

  • Keywords has-patch commit removed

This is broken in the case of links because the POST gets redirected by the code in link-manager.php:

} elseif ( ! empty( $_REQUEST['_wp_http_referer'] ) ) {
	 wp_redirect( remove_query_arg( array( '_wp_http_referer', '_wpnonce' ), stripslashes( $_SERVER['REQUEST_URI'] ) ) );
	 exit;
}

comment:13 westi3 years ago

Broken by [15491]

westi3 years ago

comment:14 westi3 years ago

  • Owner changed from scribu to westi
  • Status changed from reviewing to accepted

comment:15 garyc403 years ago

  • Keywords has-patch added

comment:16 paulwillen3 years ago

Hi, Just a quick question aside.

Whats the difference between a .patch file and a .diff file? And which should I use in which situation? Is it documented anywhere?

comment:17 nacin3 years ago

No difference. Trac supports both for syntax highlighting, so it's personal preference. I use diff, but I used to use patch. I probably transitioned around the time I moved to Mac and got used to typing svn diff on the command line. It's also one less character :-)

The extensions come from the linux commands, diff (to make a differences/patch file) and patch (used to apply a differences/patch file).

comment:18 westi3 years ago

I've tried putting a wp_die in the branch of code that is being changed.

And I can't trigger it at all.

comment:19 filosofo3 years ago

fix-no-js-please-everywhere-use-get.14927.diff uses the get method for the media form, just like the other admin forms. So it keeps the searched term in the search input element value and says "searched for x."

Testing bulk action delete seems to work fine with patch applied and js disabled.

comment:20 follow-up: filosofo3 years ago

OK, it was pointed out to me that we switched to POST to avoid overly long GET requests for bulk edit.

How about switching the search to a separate form, like the tags edit page?

comment:21 in reply to: ↑ 20 westi3 years ago

Replying to filosofo:

OK, it was pointed out to me that we switched to POST to avoid overly long GET requests for bulk edit.

How about switching the search to a separate form, like the tags edit page?

We should - but not for 3.1

comment:22 westi3 years ago

(In [17307]) Switch back to GET from REQUEST for these so the non js searches work again. See #14927

comment:23 scribu3 years ago

As I'm sure you know, this is inconsistent with edit.php et. al and the search field isn't populated with the current value.

westi3 years ago

Fix search box population

comment:24 westi3 years ago

That fixes the search box population when no-js

Testing with something like

http://trunk.domain/wp-admin/upload.php?s=gif

And then searching for something else previously would populate the search box with the value from the url even though it wasn't being used anymore

comment:25 westi3 years ago

(In [17309]) Switch _admin_search_query to use $_REQUEST so as to work for no-js searches sent over POST as well as url based searches over GET. See #14927

comment:26 nacin3 years ago

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

Works great.

comment:27 ocean903 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

See 14927.show.search.results.text.patch. The sentence is missing in the no-js version.

comment:28 westi3 years ago

Good Catch.

I missed this because I was testing with both GET and POST set search requests at the same time so the check in GET worked and then used {{{get_search_query}} to pull the data from POST

Version 0, edited 3 years ago by westi (next)

comment:29 westi3 years ago

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

(In [17311]) Switch to REQUEST so the search always shows up when no-js not just when you have an old GET request in the url too. Fixes #14927 props ocean90

Note: See TracTickets for help on using tickets.