While working on #1269734: Add form name class to forms for styling purposes I discovered that search-block-form is a strange case. Although search-block-form.tpl.php is a block template, the 'render element' in search_theme() is set to 'form'. This causes the markup to be generated by:

1. search_block_view() does a drupal_get_form() to get the form.
2. template_preprocess_search_block_form() creates a $variables['search_form'] which contains all of the rendered form children.
3. search-block-form.tpl.php prints $search_form, wrapped in the block wrappers.
4. Somewhere the <form ...> ... </form> is being generated and wrapped around the output of search-block-form.tpl.php

The problem is that the $form root element from the drupal_get_form() call in search_block_view() is never being rendered, and therefore not being rendered in a way consistent with all other forms in Core.

Files: 
CommentFileSizeAuthor
#40 search.patch455 bytesCliff
PASSED: [[SimpleTest]]: [MySQL] 33,144 pass(es).
[ View ]
#30 remove-search-block-form-tpl_4.patch3.64 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 33,055 pass(es).
[ View ]
#21 search-title.jpg2.7 KBandypost
#21 search-no-title.jpg1.86 KBandypost
#18 remove-search-block-form-tpl_5.patch3.64 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 32,861 pass(es).
[ View ]
#14 1285364-search-form.patch3.98 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 32,868 pass(es).
[ View ]
#13 remove-search-block-form-tpl_4.patch3.64 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 32,888 pass(es).
[ View ]
#12 remove-search-block-form-tpl_3.patch3.63 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 32,861 pass(es).
[ View ]
#6 remove-search-block-form-tpl_2.patch3.31 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 33,059 pass(es).
[ View ]
#3 remove-search-block-form-tpl.patch3.65 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 33,071 pass(es).
[ View ]
#1 rm-search-block-form.patch3.05 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 33,071 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 33,071 pass(es).
[ View ]

This should remove the search-block-form.tpl.php and render the block / form properly. The invisible title is missing now, so we need to address that.

We should be able to do the hidden title with:

block['subject'] = t('Search');

And then by adding

$title_attributes['class'][] = 'element-invisible';

But, I'm not quite sure where to add the variable to the template, I know that $title_attributes is already defined in block.tpl.php

StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 33,071 pass(es).
[ View ]

This patch:

- Removes verything related to search-block-form.tpl.php
- Renders the search_block_form completely within the block
- Adds the 'subject t('Search form') to the block
- Hides the block title (subject) with element-invisible

Title:Correct form rendering inconsistency with search_block_formRemove search-block-form template for form rendering consistency
Issue tags:+html5

I don't think we should hide the title by default. The UI would be too inconsistent. People would get confused when they override the subject and never see anything. Also the whole reason to do this is for consistency. Let's make the title "Search" and if themes want to hide it, let them worry about it.

We can make adjustments to the core themes where needed in this patch, like for when the block is placed in the header, if it's necessary. I know Bartik has code in place for this already:

<?php
function bartik_preprocess_block(&$variables) {
 
// In the header region visually hide block titles.
 
if ($variables['block']->region == 'header') {
   
$variables['title_attributes_array']['class'][] = 'element-invisible';
  }
}
?>

StatusFileSize
new3.31 KB
PASSED: [[SimpleTest]]: [MySQL] 33,059 pass(es).
[ View ]

@jacine

Thanks for the feedback, I've re-rolled. I was just copying the behavior of the current template. Actually, the current template hard codes t('Search form'). I definitely like your suggestion better.

What's about themes that directly prints search form into page.tpl?

@andypost

I'm not quite sure I understand your question. If you are asking what happens to themes that print $drupal_render(drupal_get_form('search_block_form')) then.

1. They are not following proper conventions and it doesn't matter if they fail in D8.

2. There, as far as I know, should be no change. There is no change to the search_block_form in this patch, only changes to the search block that contains the form.

Status:Needs review» Needs work

@Everett Zufelt make sense, container-inline wrapper could be added in page|block template

+++ /dev/nullundefined
@@ -1,37 +0,0 @@
-?>
-<div class="container-inline">
<?php if (empty($variables['form']['#block']->subject)): ?>
-    <h2 class="element-invisible"><?php print t('Search form'); ?></h2>
<?php endif; ?>
<?php print $search_form; ?>
-</div>

I see the only purpose of this template to add the "inline" class

7 days to next Drupal core point release.

@andypost

Are you suggesting that I add to the patch:

<?php
/**
* Implements hook_preprocess_block().
*/
function search_preprocess_block(&$variables) {
  if (
$variables['block']->module == 'search' && $variables['block']->delta == 'form') {
   
$variables['classes_array'][] = 'container-inline';
  }
}
?>

Sure because textfield and submit button should be placed on one line

Status:Needs work» Needs review
StatusFileSize
new3.63 KB
PASSED: [[SimpleTest]]: [MySQL] 32,861 pass(es).
[ View ]

Adds container-inline suggested in #9 to place search field and button inline.

StatusFileSize
new3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 32,888 pass(es).
[ View ]

Moves .container-inline to content_attributes_array.

StatusFileSize
new3.98 KB
PASSED: [[SimpleTest]]: [MySQL] 32,868 pass(es).
[ View ]

probably we need tests for this because content_attributes_array for "class" does not work for blocks (FF, Chrome tested) because of class alredy defined in template
<div class="content"<?php print $content_attributes; ?>>

So I changed search_box() probably we need check this with

<?php
if ($form_id' == 'search_block_form') {
  $form['
#attributes']['class'] = 'container-inline';
}

?>

Also added back "element-invisible" for block title

EDIT suppose we should file another issue about block.tpl.php about useless $content_attributes for default block template, I can't find similar, but this should be fixed and backported to D7

Status:Needs review» Needs work

Ugh! Sorry, I forgot about the fact that class is not allowed in $content_attributes. That really bugs me to no end and I really, really hope we can fix this in D8. The form itself is a good candidate though.

I really don't think we need to make anything invisible here. IMO, we tend to abuse .element-invisible and are introducing little Easter eggs all over the place. In this case, if we make the title invisible we introduce a regression. Right now, you can change the title in the block edit UI and it will appear. If we do this, it wont. We don't do this for other blocks either, so it's inconsistent.

Status:Needs work» Needs review

@andypost

1. I think that we should leave the block title visible by default, if themes want to hide it they can.
2. I like 'Search' better than 'Search form', but this is a personal preference.
3. Can you please explain why using $variables['content_attributes_array']['class'][] == 'container-inline'; isn't working? It is definitely generating the correct markup.

Okay, I see now... class="content" is hard coded in the tpl. I'll open a followup, because that is all kinds of wrong!

StatusFileSize
new3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 32,861 pass(es).
[ View ]

This patch:

- Removes the search-block-form template
- Adds class .container-inline to the form
- Adds subject (title) 'Search' to the block

Followup is that themes will need to hide / alter the title to make it go away / be invisible.

I've filed patch & test for block content class #1286532: Fix $content_attributes_array does not work for block default template
Feel free to rework if I wrong

+++ b/modules/search/search.moduleundefined
@@ -1007,6 +1004,7 @@ function search_form($form, &$form_state, $action = '', $keys = '', $module = NU
+  $form['#attributes']['class'] = 'container-inline';

Still not sure about setting this directly for all forms. Search uses this with hook_forms()

7 days to next Drupal core point release.

StatusFileSize
new1.86 KB
new2.7 KB

Screens:
1) By default in bartik we have block title now visible
2) Assinging < none > form block title helps

The patch in #13 is dependent on #1286532: Fix $content_attributes_array does not work for block default template and is what should be tested.

#13: remove-search-block-form-tpl_4.patch queued for re-testing.

#1286532: Fix $content_attributes_array does not work for block default template has been committed, so patch #13 should be good to go now.

Status:Needs review» Reviewed & tested by the community

#13 works fine - screens from #21 are valid

Follow up should be for Bartik maintainer to decide if they want to hide the title, I agree with Jacine that it should not be hidden.

<?php
function bartik_preprocess_block(&$variables) {
  if (
$variables['block']->module == 'search' && $variables['block']->delta == 'form') {
   
$variables['title_attributes_array']['class'][] = 'element-invisible';
  }
}
?>

Looks all very good to me and brings back some consistency - if someone wants to remove/change the title in Bartik they can do so, hiding it by default feels quite wrong and inconsistent with core.

So yes, leave the title to show in Bartik, it will still be hidden in the header region for style purposes, but everywhere else we let it show. +1.

Sweet! Thanks everyone. ;)

Also, just want to note for future reference that this patch got emmajane and the TNT crew's blessing as well in IRC.

Status:Reviewed & tested by the community» Needs work

It looks like #13 is the right patch to commit here, but please upload that so it's the last patch on this issue. Needs work in the sense that someone needs to attach the patch ;)

Status:Needs work» Needs review
StatusFileSize
new3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 33,055 pass(es).
[ View ]

Attaching patch from #13.

Status:Needs review» Reviewed & tested by the community

Setting back to RTBC.

Status:Reviewed & tested by the community» Fixed

Really lovely to see a hook_theme() and template get replaced by a three line preprocess. Committed to 8.x.

Status:Fixed» Active

Curious if we should document that we just removed a template.

Status:Active» Needs work
Issue tags:+Needs change record

Someone needs to create a change notice that describes this change.

Also, there needs to be a minor patch addition: the function search_box() 's docblock still links to this removed theme template file, and that link will now be broken:
http://api.drupal.org/api/drupal/modules--search--search.module/function...

(I guess the API site hasn't updated to the new code yet).

@jhodgdon

Can you please let me know where I got to learn about / create change notifications?

Sure! The only place they're really described that I know of is in the "documentation gate" section of the Drupal 8 "Gates" page. The description is in the last row of the table in the Documentation section.

Basically, they are a new content type on d.o. To add a new one:
http://drupal.org/node/add/changenotice

To see a list of the existing change notices for Drupal core:
http://drupal.org/list-changes/drupal

Status:Needs work» Fixed
Issue tags:-Needs change record

Added change notification http://drupal.org/node/1294082

Status:Fixed» Needs work
Issue tags:+Novice

I think we also still need a small doc patch, since there is still one mention of search-block-form.tpl.php in search.module, and that has been removed.

This follow-up patch would be a good Novice project.

oops, forgot that part, and agree re: #34:

Also, there needs to be a minor patch addition: the function search_box() 's docblock still links to this removed theme template file, and that link will
now be broken:
http://api.drupal.org/api/drupal/modules--search--search.module/function...

(I guess the API site hasn't updated to the new code yet).

Status:Needs work» Needs review
StatusFileSize
new455 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,144 pass(es).
[ View ]

Adding patch to achieve #38.

Status:Needs review» Reviewed & tested by the community

Looks good. Simple patch to remove the reference to search-block-form.tpl.php from function search_box().

Status:Reviewed & tested by the community» Fixed

Well spotted. Committed and pushed.

Automatically closed -- issue fixed for 2 weeks with no activity.