Make WordPress Core

Opened 13 years ago

Closed 11 years ago

#16541 closed defect (bug) (fixed)

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

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

Description

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 13 years ago.
Silly output buffering
16541.compact.diff (561 bytes) - added by kawauso 13 years ago.
16541.2.diff (1.2 KB) - added by garyc40 13 years ago.
pass the form content through get_search_form filter no matter what $echo is

Download all attachments as: .zip

Change History (23)

@kawauso
13 years ago

Silly output buffering

#1 @kawauso
13 years ago

  • Keywords has-patch added

@garyc40
13 years ago

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

#2 @garyc40
13 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.

#3 @webraket
13 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.

If this is not possible because of compatibility issues, I wish get_search_form() or locate_template() becomes pluggable...

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.

Last edited 13 years ago by webraket (previous) (diff)

#4 @apljdi
13 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.

Feedback?

#5 @kawauso
13 years ago

  • Keywords dev-feedback added

#6 follow-up: @shawnkhall
12 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.

#7 in reply to: ↑ 6 @coffee2code
12 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.

#8 follow-ups: @chipbennett
12 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?

#9 in reply to: ↑ 8 @kawauso
12 years ago

Replying to chipbennett:

why not use file_get_contents()

It doesn't do any parsing for PHP tags.

#10 @Chouby
12 years ago

  • Cc Chouby added

#11 @HumanNic
12 years ago

  • Cc HumanNic added

#12 in reply to: ↑ 8 @bitacre
11 years 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.

#13 @nacin
11 years ago

This is exactly what output buffering is made for.

#14 @WraithKenny
11 years ago

+1 for 16541.2.diff

#15 @mark-k
11 years 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.

#17 @nacin
11 years ago

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

#18 @SergeyBiryukov
11 years ago

  • Keywords commit added

#19 @nacin
11 years ago

Looks good to me.

#20 @SergeyBiryukov
11 years 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.