WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

Last modified 8 months ago

#14851 closed enhancement (wontfix)

Add ¨searchform-{name}.php¨ support

Reported by: ramiy Owned by: nacin
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Themes Keywords: has-patch dev-feedback needs-testing
Focuses: template Cc:

Description

some include tags like get_header(), get_footer() and get_sidebar() accept $name parametter to include header-{name}.php,footer-{name}.php and sidebar-{name}.php.

but the search form include tag - get_search_form() - does not accept $name parametter. currently it includes only searchform.php.

if this function will accept $name parametter, it would be easier to include other type search forms. this thecnic is very useful to sites that have several search form formats - for example one in the header, one in sidebar and one for footer. or an advense search form with categry select and post type select boxes to specific pages.

Attachments (3)

14851.patch (1.9 KB) - added by ramiy 4 years ago.
14851.2.patch (1.2 KB) - added by WraithKenny 2 years ago.
14851.3.patch (1.0 KB) - added by terminalpixel 8 months ago.

Download all attachments as: .zip

Change History (25)

comment:1 @ramiy5 years ago

  • Cc ramiy added

comment:2 @greenshady5 years ago

I can definitely see the use in this. We have multiple search forms that only search within specific post types for different sections of our site. Currently, we just write the different forms using conditional tags or hard coding the forms into various templates.

comment:3 @ramiy5 years ago

The new get_search_form() should look like get_header(), get_footer() and get_sidebar() functions.

function get_search_form( $name = null ) {
	do_action( 'get_search_form', $name );

	$templates = array();
	if ( isset($name) )
		$templates[] = "searchform-{$name}.php";

	$templates[] = "searchform.php";

	// Backward compat code will be removed in a future release
	if ('' == locate_template($templates, true))
		load_template( ABSPATH . WPINC . '/theme-compat/searchform.php');
}

And we need to add new file "wp-includes/theme-compat/searchform.php" for backward compat:

<form role="search" method="get" id="searchform" action="<?php home_url( '/' ); ?>">
    <div><label class="screen-reader-text" for="s"><?php __('Search for:'); ?></label>
        <input type="text" value="<?php echo get_search_query(); ?>" name="s" id="s" />
        <input type="submit" id="searchsubmit" value="<?php esc_attr__('Search'); ?>" />
    </div>
</form>

that is it !

comment:4 @nacin5 years ago

We would not deprecate the HTML component. The theme-compat folder is for overly minimal themes that needed a header/footer/etc but failed to include such template in the folder.

On the other hand, get_search_form() encourages standardization and simplicity the way comment_form() does. With the option of it being overridden with a template, of course. We actually removed searchform.php from Twenty Ten because it was nearly identical to core's implementation.

comment:5 follow-up: @ramiy5 years ago

ok nacin, then how about:

function get_search_form( $name = null) {
	do_action( 'get_search_form', $name );

	$templates = array();
	if ( isset($name) )
		$templates[] = "searchform-{$name}.php";

	$templates[] = "searchform.php";

	// if file doesn't exist, the default search form will be displayed
	if ('' == locate_template($templates, true))
		echo '<form role="search" method="get" id="searchform" action="' . home_url( '/' ) . '" >
			<div><label class="screen-reader-text" for="s">' . __('Search for:') . '</label>
			<input type="text" value="' . get_search_query() . '" name="s" id="s" />
			<input type="submit" id="searchsubmit" value="'. esc_attr__('Search') .'" />
			</div>
			</form>';
}

comment:6 @ramiy5 years ago

  • Cc ramiy removed
  • Owner set to nacin
  • Status changed from new to reviewing

comment:7 @nacin4 years ago

  • Keywords needs-patch added; need-patch removed
  • Milestone changed from Awaiting Review to Future Release

@ramiy4 years ago

comment:8 @ramiy4 years ago

  • Cc ramiy added
  • Keywords has-patch dev-feedback added; needs-patch removed

comment:9 @chipbennett3 years ago

  • Cc chip@… added
  • Keywords changed from has-patch, dev-feedback to has-patch dev-feedback

Related: #19579

comment:10 @retlehs2 years ago

  • Cc retlehs added

comment:11 @WraithKenny2 years ago

a bigger flaw with this patch is that it destroys the ability to set echo = false.

comment:12 @ramiy2 years ago

It can be fixed. We can check is_bool( $name ) for legacy.

comment:13 in reply to: ↑ 5 @ramiy2 years ago

The patch with legacy check to return the search form:

function get_search_form( $name = null) {
	$default_search_form = '<form role="search" method="get" id="searchform" action="' . home_url( '/' ) . '" >
		<div><label class="screen-reader-text" for="s">' . __('Search for:') . '</label>
		<input type="text" value="' . get_search_query() . '" name="s" id="s" />
		<input type="submit" id="searchsubmit" value="'. esc_attr__('Search') .'" />
		</div>
		</form>';

	// Legacy check to return the search form
	if ( ( is_bool( $name ) ) && ( $name = false ) ) {
		return $default_search_form;
	}

	do_action( 'get_search_form', $name );

	$templates = array();
	if ( isset( $name ) )
		$templates[] = "searchform-{$name}.php";

	$templates[] = "searchform.php";

	// if file doesn't exist, the default search form will be displayed
	if ( '' == locate_template( $templates, true ) )
		echo $default_search_form;
}

comment:14 @WraithKenny2 years ago

(There are several pending commits on this function, so you should probably refresh the patch with those taken into account.)

Echoing isn't really 'legacy' as it's still going to be the main usage case for most people. The function should return as it currently does if no argument is passed, or if a bool is passed.

What you really want is to 'overload' the arg, so check if it's a string (rather then for a bool) and put your new functionality in the conditional: assemble $templates, and set $echo as false (to match the other get_* functions). Leave the rest of the function as is ('ninja' style).

Then update the docs that the arg is overloaded taking either bool or string.

Hope that helps

@WraithKenny2 years ago

comment:15 @WraithKenny2 years ago

  • Keywords needs-testing added

@ramiy would you mind taking a look at / testing the latest patch and see if that works for you?

comment:16 @nacin16 months ago

  • Component changed from Template to Themes
  • Focuses template added

@terminalpixel8 months ago

comment:17 @terminalpixel8 months ago

Updated the patch by @WraithKenny to work with latest trunk (4.1)

comment:18 @obenland8 months ago

I'm not a big fan of this, for two reasons:

  • It would be extremely confusing to use the same function argument for two different purposes, with different contexts, and different expected data types.
  • I'm not sure the argument to make it "easier to include other type search forms" is valid.

How many different types of search forms do theme authors really need? Beyond contextual classes in parent elements? The current form is fully style-able, so it would come down to varying input elements.

I agree, it is inconsistent with get_header(), get_footer(), and get_sidebar(). But that compatibility would come at a high price. Especially since get_template_part() can mitigate that need:

get_template_part( 'search-form', 'header' );
get_template_part( 'search-form', 'sidebar' );
get_template_part( 'search-form', 'footer' );

comment:19 @nacin8 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

After [13048] (which came before this ticket was opened) this came tough to do without making it particularly confusing. I further agree with the argument that it is unnecessary for two search forms to exist, and also that get_template_part() more or less could solve for this use case anyway.

comment:20 @terminalpixel8 months ago

I agree with it being rather unnecessary with get_template_part() available.

Thanks for helping me to start contributing though!

comment:21 follow-up: @ramiy8 months ago

Guy's you can't say that two search forms are unnecessary, i have a site with 3 different search forms. I've seen sites that use different search forms in the header, sidebar and the footer. You can also create different search forms for desktop and mobile devices. Or create simple search form and another advance search with extra fields.

And as for using get_template_part(), you can say all Include Tags are unnecessary ( get_header(), get_footer(), get_sidebar() ). We can replace all of them with get_template_part() but we don't do that. Each tag has it's purpose, so is get_search_form(), i just want to extend it like all other include functions.

comment:22 in reply to: ↑ 21 @nacin8 months ago

Replying to ramiy:

And as for using get_template_part(), you can say all Include Tags are unnecessary ( get_header(), get_footer(), get_sidebar() ). We can replace all of them with get_template_part() but we don't do that.

And we almost did, but it complicated because of the existing do_action() calls.

Each tag has it's purpose, so is get_search_form(), i just want to extend it like all other include functions.

We can't, though, because it has an existing second argument. So I'm inclined to not do anything here for now.

Note: See TracTickets for help on using tickets.