Consolidate search-result.html.twig and search-results.html.twig

Would this cause it to be more difficult in case themers wanted to seperate these templates out?

Forgive my ignorance. I'm just starting to try to understand the twig system.

I've merged the 2 template files into one and the 2 preprocess functions into one, please review.

I've kept the function that prepares the results as a separate function, is there a naming or placement convention for it? (rather new to Drupal here)

@chrisjlee, with Twig you would be able to separate the templates without needing to alter the php code, you would do

{% for search_result in search_results %}
  {% include "search-result.html.twig" %}
{% endfor %}

and the variable would be available in the search-result.html.twig file that you would create. I'm not sure how you would include the new template file with the way Twig is set up in Drupal, the above didn't seem to work for me.

Patch created as part of Florida DrupalCamp 2013

After some exhaustive research, it looks like this could be a problem with the test. The search_extra_type module has some hooks in it that may be problematic at line 58: search_extra_type_search_page. Continuing to investigate if the other failures might have to do with the test.

I am working on this.

Here is a completely new patch. I ran most of the search module tests and I think this should work. I didn't really start from the patch above, so an interdiff is not all that useful.

added related issue

I just added an issue summary.

Note that if we do not decide to go this route, we need to do
#1517032: Update search-result.tpl.php code comments to reflect new $info_split default keys
for 8.x.

Also this will conflict with
#2105609: Convert search_embedded_form_form to a Controller
so whichever gets in first, the other will need a new patch.

This looks great! I think we should take the consolidation of this issue a little farther though and just use an item list: item_list__search_results. I'm going to create an issue to extend the item list function to include an "#empty" option like theme_table() does.

#2120807: Add empty option to item_list

OK.

Here's a completely new approach (again, and interdiff to the previous patch would not be useful).

This patch depends on 3 issues being taken to completion, so it won't apply now and I'm leaving this at "needs work" for the moment. These are:
#2105609: Convert search_embedded_form_form to a Controller
#1987806: Convert search_view() to a new style controller
#2120807: Add empty option to item_list

The idea here is that the search-results.tpl.php file is replaced by using a theme('item_list') with theme suggestions. Once #2120807: Add empty option to item_list is done, theme('item_list') allows us to pass in the "empty" text (for the no search results case), so theme overrides can be used to customize the output as desired, for both the has and doesn't have results cases. Which was basically the purpose of the results template.

The search-result template is retained, but the LI wrapper is removed. So it just formats each result items, and the list of them is themed with an item_list theme function.

One other change: the markup for the no results case is changed slightly: there's a heading of Search Results even if there are no results.

I'm going to go ahead and upload this patch, and then I'll run the Search module tests locally and see what happens.

Looks great! Thanks @jhodgdon! Changing status to postponed since this issue is dependent on other issues. Updated issue summary as well.

added issue summary

Updated issue summary.

update plans

The dependent issues are finally all taken care of. So, I'm un-postponing this issue, and if the patch doesn't start its test, I'll ping it.

Lovely. Looks like I'll have to re-upload the patch, and then file a D7 d.o regression issue because there is no way to launch a test on the previously-uploaded file.

This is the exact same patch file as in #13.

Huh. It doesn't look like this patch is attempting to queue for test either... any ideas?

OK it is just slow but this needs a reroll. :(

Rerolled.

The tests that are failing probably need an update. I believe they are failing because "Search Results" used to only be visible if there was at least one result, and now "Search Results" is always visible. I'm not sure when those tests were added, but they need some attention.

Actually, I take it back. It looks like the test is failing because the title "Search Results" is not being displayed on an actual search results page. This is probably an actual bug.

I don't have time to investigate this week, so if someone else wants to fix this one, that would be fine. Otherwise, I may be able to get back to it next week.

Oh wait. The text changed from "Search results" to "Search Results". This should fix it.

So the 'Search Results' title is displayed even if there are no results, that's probably okay. But the way things are set up now we have two <h3>'s in a row which seems bad. Search Results before this patch was a <h2> which is more difficult to achieve with theme_item_list() of course. Not sure the best way forward but I think that needs to at least be discussed.

<h3>Search Results</h3><h3>Your search yielded no results.</h3>

+++ b/core/modules/search/templates/search-result.html.twig
@@ -3,9 +3,7 @@
- * This template renders a single search result and is collected into
- * search-results.html.twig. This and the parent template are
- * dependent to one another sharing the markup for ordered lists.
+ * This template renders a single search result.

Can we document here that theme_item_list__search_results() (and maybe them_item_list__search_results__PLUGIN_ID()) can be overwritten to change the outer markup? Or document that this template is rendered within an item_list?

The headers are no worse than the mess they were in before this change... fixing them might be possible...

Documentation on the result twig sounds linke a fine idea.

This might be a silly idea but can we just make the "Your search yielded no results" a p tag?

The problem is that it's actually a header for other information that comes after, but we could possibly make it an H4.

So... Let's step back and take a look at the markup.

Without the patch, we currently have in D8 is something like this if there are no results:

<h1 id="page-title">Search</h1>
(form)
(open some divs for the regions etc.)
<h2>Your search yielded no results</h2>
<ul>(no results explanations)</ul>
(end of divs)

And if there are results:

<h1 id="page-title">Search</h1>
(form)
(open some divs for the regions etc.)
<h2>Search results</h2>
<ol class="search-results">
<li><h3>Result 1 title</h3><div>(snippet etc.)</div></li>
<li><h3>Result 2 title</h3><div>(snippet etc.)</div></li>
</ol>

With this patch, this changes to:

<h1 id="page-title">Search</h1>
(form)
(open some divs for the regions etc.)
<h3>Search results</h3>
<h3>Your search yielded no results</h3>
<ul>(no results explanations)</ul>
(end of divs)

And if there are results:

<h1 id="page-title">Search</h1>
(form)
(open some divs for the regions etc.)
<h3>Search results</h3>
<ol class="search-results">
<li><h3>Result 1 title</h3><div>(snippet etc.)</div></li>
<li><h3>Result 2 title</h3><div>(snippet etc.)</div></li>
</ol>

So it looks like the correct fix for this patch, which would preserve as much as possible what was being done before, would be to put the Search Results header into an H2 instead of H3. The reason it changed to an H3 was because we are putting this into theme_item_list() as the #title attribute. So we just need to do the title outside the list. Having it be an H3 is wrong for this case.

Attached patch fixes those Search Results headings to be h2, and also adds documentation as suggested above.

30: 1926344-30.patch queued for re-testing.

Looks like this one needs a reroll.

This is a straight reroll of the previous patch. Maybe we could get it reviewed?

I would love to!

Doh. This should work better. Interdiff - from last patch, in SearchController:

-      '#theme' => array('item_list__search_results__' . $plugin_id, 'item_list__search_results'),
+      '#theme' => array('item_list__search_results__' . $plugin->getPluginId(), 'item_list__search_results'),
 

Whoops. Guess I should have at least tested the reroll manually. Good thing we have tests. :)

I compared markup and it's looking good overall! I think the only thing we need to fix is the classes we are losing which will be helpful for styling. search-results and <plugin_id>_search-results disappear.

Before:

<h2>Search results</h2>
  <ol class="search-results node_search-results">
    <li >

After:

<h2>Search Results</h2><div class="item-list"><ol><li>

So I think we need to add these back to the item-list div or more ideally to the ol.

Did profiling for fun and performance looks comparable.

=== 8.x..8.x compared (52e417f8e04a5..52e4189e0e4fe):

ct  : 133,634|133,634|0|0.0%
wt  : 434,343|433,973|-370|-0.1%
cpu : 414,199|413,848|-351|-0.1%
mu  : 19,059,136|19,059,136|0|0.0%
pmu : 19,203,472|19,203,472|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e417f8e04a5&...

=== 8.x..search-1926344-39 compared (52e417f8e04a5..52e418f395327):

ct  : 133,634|133,553|-81|-0.1%
wt  : 434,343|434,375|32|0.0%
cpu : 414,199|414,385|186|0.0%
mu  : 19,059,136|19,023,896|-35,240|-0.2%
pmu : 19,203,472|19,168,568|-34,904|-0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e417f8e04a5&...

Tagging as novice to add the class to the OL. Hint: look at theme_item_list API docs :)

Search results, now with classes :)

Patch added

Thanks @maggo, that looks good at a glance, interdiffs are especially helpful in cases like this but I diffed the two patches and the changes look good.

+++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
@@ -89,7 +89,30 @@ public function view(Request $request, SearchPageInterface $entity, $keys = '')
+      '#attributes' => array(
+        'class' => array(
+          'search-results',
+          $plugin->getPluginId() . '_search-results'
+        )
+      )

We just need some trailing/dangling commas here per https://drupal.org/coding-standards#array.

Like this:

'#attributes' => array(
  'class' => array(
    'search-results',
    $plugin->getPluginId() . '_search-results',
  ),
),

+++ b/core/modules/search/templates/search-result.html.twig
@@ -3,9 +3,10 @@
+ * rendered using theme('item_list'), with suggestions of:

I forgot to mention this in my review but we shouldn't be referencing theme() like this in documentation, especially given #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system. '#theme' => 'item_list' would be more appropriate.

Done!

Thanks @maggo.

1. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
   @@ -94,7 +94,7 @@ public function view(Request $request, SearchPageInterface $entity, $keys = '')
   -      '#theme' => array('item_list__search_results__' . $plugin->getPluginId(), 'item_list__search_results'),
   +      '#theme' => 'item_list',
   

   This line was fine - please revert this change and make the requested change in search-result.html.twig instead.

2. +++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
   @@ -104,7 +104,7 @@ public function view(Request $request, SearchPageInterface $entity, $keys = '')
   -          $plugin->getPluginId() . '_search-results'
   +          $plugin->getPluginId() . '_search-results',
            )
          )
        );
   

   Almost! We need two more trailing commas on the lines that only have a closing parenthesis.

I can work on this change.

Added a patch with the changes Cottser suggested, along with an interdiff.

Thanks @mdrummond!

After reviewing this again a couple things stood out to me.

1.

+++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
@@ -89,7 +89,30 @@ public function view(Request $request, SearchPageInterface $entity, $keys = '')
+      '#markup' => '<h2>' . $this->t('Search Results') . '</h2>',

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchPreprocessLangcodeTest.php
@@ -87,7 +87,7 @@ function testPreprocessStemming() {
-    $this->assertText('Search results');
+    $this->assertText('Search Results');

@@ -95,7 +95,7 @@ function testPreprocessStemming() {
-    $this->assertText('Search results');
+    $this->assertText('Search Results');

I'm not exactly sure why we're changing the "Search results" interface text to "Search Results". Minor but it is a change that seems unrelated.

2. Now that the search results header is split out, we could hide it when there are no search results, this is the current behaviour and we can maintain it. Then the only difference will be that 'Your search yielded no results' will be an h3 instead of an h2 which is probably fine :)

Before:

After:

Oh, and I forgot to mention the class names on the ol are not quite right.

Before:

<ol class="search-results node_search-results">
    <li >

After:

<ol class="search-results node_search_search-results"><li>

So this code should just be adding -results, not _search-results to the plugin ID.

+++ b/core/modules/search/lib/Drupal/search/Controller/SearchController.php
@@ -89,7 +89,30 @@ public function view(Request $request, SearchPageInterface $entity, $keys = '')
+          $plugin->getPluginId() . '_search-results',

Wow, thanks for all the work on this! It looks like we just need a couple more small changes to the current patch. Is anyone up for making those changes or should I step back in?

This new patch addresses the issues in #52 (classes on the OL are now search-results node_search-results) and #51 (see new screenshots, made with Stark theme).

With results:

No results:

Thanks @jhodgon!

This looks great. I really like how this simplifies overriding search results and leverages item_list.

One idea for a small and Novice-able follow-up would be to rename search.pages.inc to search.theme.inc and update the @file now that it only contains theme API hook implementations (preprocess and suggestions).

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchInterface.php
@@ -75,7 +75,9 @@ public function execute();
-   *   An array of search_result render arrays.
+   *   An array of render arrays of search result items (generally each item
+   *   has '#theme' set to 'search_result'), or an empty array if there are no
+   *   results.

Love the docs change here!

Committed and pushed to 8.x. Thanks!

This broke head so I've reverted it.

Committed 580dd44 and pushed to 8.x. Thanks!

Drupal test run
---------------

Tests to be run:
 - Config settings form (Drupal\search\Tests\SearchConfigSettingsFormTest)

Test run started:
 Friday, February 7, 2014 - 10:58

Test summary
------------

Config settings form 267 passes, 3 fails, and 0 exceptions

It broke because it's possible to have the no result test on a page with results :) See below.

54: 1926344-54.patch queued for re-testing.

Whoops! Apparently conflicted with another search.module patch. I'll see about the failures.

Cottser pinged me in IRC. Apparently the problem really was that a commit to #1939062: Convert theme_item_list() to Twig around the same time broke this patch. The plan is to fix that issue and then this one should be able to test green.

Ok sorry about that, #2191323: item-list.html.twig always shows empty text is now committed, so we should be able to proceed here.

54: 1926344-54.patch queued for re-testing.

Great! OK the next thing is to retest the patch above and see if that fixed the test failure or if something needs to be refactored.

Oh, cottser just did that, crosspost!

Tests now pass again, since that other issue was fixed, so back to RTBC.

This patch conflicted with #2183923: Break the circular dependency in EntityManager. Considering I reverted it here is a reroll.

68: 1926344.68.patch queued for re-testing.

Back to rtbc - random test fail.

Committed 4fb874b and pushed to 8.x. Thanks!

Thanks @alexpott!

Hooray!