Make WordPress Core

Opened 3 years ago

Closed 14 months ago

#16541 closed defect (bug) (fixed)

get_search_form() ignores $echo argument if searchform.php exists

Reported by: kawauso Owned by: SergeyBiryukov
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.0
Component: Template Keywords: has-patch commit
Focuses: Cc:


If a theme has searchform.php present, get_search_form() uses require() regardless of the $echo value.

As per discussion with nacin in IRC, the only workaround for this would be use output buffering (which would also allow for searchform.php to be passed through the 'get_search_form' filter).

Attachments (3)

16541.diff (655 bytes) - added by kawauso 3 years ago.
Silly output buffering
16541.compact.diff (561 bytes) - added by kawauso 3 years ago.
16541.2.diff (1.2 KB) - added by garyc40 3 years ago.
pass the form content through get_search_form filter no matter what $echo is

Download all attachments as: .zip

Change History (23)

kawauso3 years ago

Silly output buffering

kawauso3 years ago

comment:1 kawauso3 years ago

  • Keywords has-patch added

garyc403 years ago

pass the form content through get_search_form filter no matter what $echo is

comment:2 garyc403 years ago

Does the content of the search form template (if it exists) need to pass through get_search_form filter? Or only the default $form?

My patch passes the content of the search form template through the filter instead of returning prematurely.

comment:3 webraket3 years ago

I would like to disable the include of searchform.php (sitewide on a multi-site install with 100+ themes) and pass a custom $form value trough the filter with a plugin.

With the latest patch provided by garyc40, at least I don't have to modify the core however searchform.php is still included and the outbut buffering is probably a waste of resources.

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

comment:4 apljdi3 years ago

I agree. The current design of get_search_form() is very strange. The echo parameter is present but only works with the default content. One solution is the output buffering as in the patch above, but I agree with the sentiment that it is a waste of resources. A workaround is to pass the form through the get_search_form filter but again, that is a waste of resources as get_search_form() will look for, and possibly require, a file only to have it essentially discarded in favor of the form passed through the filter. My suggestion is to drop the file search and include altogether and pass the form only via the filter. My only concern is that this method could be awkward with large and complicated search forms.


comment:5 kawauso3 years ago

  • Keywords dev-feedback added

comment:6 follow-up: shawnkhall2 years ago

  • Cc shawnkhall added

It would be good if locate_template were hookable. Immediately inside the locate_template function it should have something like this:

$template_names = apply_filter( 'locate_template', $template_names );

This would enable plugins and themes to bypass behavior of certain core functions, such as get_search_form() without having to rewrite core.

comment:7 in reply to: ↑ 6 coffee2code2 years ago

Replying to shawnkhall:

It would be good if locate_template were hookable.

See #13239. Specifically, patch 13239.c2c.4.diff. Nacin seemed close to liking an earlier version of the patch.

I certainly favor that approach over output buffering.

comment:8 follow-ups: chipbennett2 years ago

  • Cc chip@… added

So, this could be a totally crazy idea (or totally stupid), but why not use file_get_contents(), and do something like:

$search_form_template = locate_template( 'searchform.php' );
if ( '' != $search_form_template ) {
	$form = file_get_contents( $search_form_template );
} else {
	// Default form handling goes here

if ( $echo ) {
	echo apply_filters('get_search_form', $form);
} else {
	return apply_filters('get_search_form', $form);

That way, both the searchform.php form, as a string, and the default form, also as a string, get passed through the get_search_form filter.

Is there some performance and/or security issue with using file_get_contents(), or something else that I'm missing?

comment:9 in reply to: ↑ 8 kawauso2 years ago

Replying to chipbennett:

why not use file_get_contents()

It doesn't do any parsing for PHP tags.

comment:10 Chouby2 years ago

  • Cc Chouby added

comment:11 HumanNic2 years ago

  • Cc HumanNic added

comment:12 in reply to: ↑ 8 bitacre19 months ago

Replying to chipbennett:

Is there some performance and/or security issue with using file_get_contents(), or something else that I'm missing?

Not necessarily, and file_get_contents() was also my first thought. It works beautifully for pure HTML forms, but if there is any PHP involved, it's a lot riskier.

It will (1) pass PHP code as plain text, and (2) require an eval() to run that code, all form a form where a 3rd party user can directly submit input. I can't think of a specific expliot, but it makes me nervous, especially when an object buffer is a viable alternative.

comment:13 nacin19 months ago

This is exactly what output buffering is made for.

comment:14 WraithKenny17 months ago

+1 for 16541.2.diff

comment:15 mark-k16 months ago

  • Cc mark@… added

Just wasted 30 minutes of my life because of this :( If there is no decision how to support the echo parameter then maybe it should be deprecated. Right now it is not reliable enough to be used in any case.

comment:17 nacin16 months ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.6

comment:18 SergeyBiryukov14 months ago

  • Keywords commit added

comment:19 nacin14 months ago

Looks good to me.

comment:20 SergeyBiryukov14 months ago

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

In 23666:

Always respect $echo argument in get_search_form(). props garyc40, kawauso. fixes #16541.

Note: See TracTickets for help on using tickets.