Updated: Comment #13

Problem/Motivation

The search result templates split apart the OL and the LI parts of an item list. This goes against the principal of keeping single HTML elements in single template files, one of our principles for cleaning up the theme layer.

Proposed resolution

Remove search-results and use the standard item_list theme template/function, with a suggestion so it can be uniquely themed for search results purposes.

Remaining tasks

Latest patch in #13 is perfect :) However it is currently blocked by:
#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

User interface changes

None.

API changes

There will be no more theme('search_results'), but only theme('search_result'). Similarly, the preprocess and other hooks will only apply to search_result.

This means only one template for themers to modify and only one preprocess hook for modules to use if they want to affect how search results are displayed, rather than two, which should be simpler. Themes can still have good control over the output via the theme suggestions on the item list that displays on the results page.

#1898448: search.module - Convert PHPTemplate templates to Twig

Files: 
CommentFileSizeAuthor
#68 1926344.68.patch8.33 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 64,217 pass(es). View
#54 noresults-54.png38.53 KBjhodgdon
#54 results-54.png48.56 KBjhodgdon
#54 interdiff.txt3.49 KBjhodgdon
#54 1926344-54.patch8.33 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 63,745 pass(es). View
#51 1926344-50-before.png27.97 KBCottser
#50 search-results-1926344-50.patch8.72 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 63,199 pass(es). View
#46 1926344-46.patch8.63 KBmaggo
PASSED: [[SimpleTest]]: [MySQL] 63,193 pass(es). View
#43 1926344-42.patch8.71 KBmaggo
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#39 1926344-39.patch8.55 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 59,901 pass(es). View
#35 1926344-35-reroll.patch8.54 KBjhodgdon
FAILED: [[SimpleTest]]: [MySQL] 60,097 pass(es), 0 fail(s), and 270 exception(s). View
#30 interdiff-26-30.txt1.72 KBjhodgdon
#30 1926344-30.patch8.56 KBjhodgdon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1926344-30.patch. Unable to apply patch. See the log in the details link for more information. View
#26 1926344-26.patch8.33 KBjhodgdon
PASSED: [[SimpleTest]]: [MySQL] 59,139 pass(es). View

Comments

chrisjlee’s picture

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.

krystalcode’s picture

Status: Active » Needs review
FileSize
5.69 KB
FAILED: [[SimpleTest]]: [MySQL] 54,741 pass(es), 18 fail(s), and 14 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, merged-search-results-tesmplates-1926344-2.patch, failed testing.

ldpm’s picture

Assigned: Unassigned » ldpm
ldpm’s picture

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.

jhodgdon’s picture

Assigned: ldpm » jhodgdon

I am working on this.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
14.08 KB
PASSED: [[SimpleTest]]: [MySQL] 59,674 pass(es). View

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.

jhodgdon’s picture

Issue summary: View changes

added related issue

jhodgdon’s picture

I just added an issue summary.

jhodgdon’s picture

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.

jhodgdon’s picture

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.

markcarver’s picture

Status: Needs review » Needs work

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.

markcarver’s picture

jhodgdon’s picture

FileSize
7.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1926344-eliminate-search-results-twig.patch. Unable to apply patch. See the log in the details link for more information. View

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.

markcarver’s picture

Status: Needs work » Postponed

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

markcarver’s picture

Issue summary: View changes

added issue summary

markcarver’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Issue summary: View changes

update plans

jhodgdon’s picture

Issue summary: View changes
Status: Postponed » Needs review

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.

jhodgdon’s picture

FileSize
7.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1926344-eliminate-search-results-twig.patch. Unable to apply patch. See the log in the details link for more information. View

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.

The last submitted patch, 13: 1926344-eliminate-search-results-twig.patch, failed testing.

jhodgdon’s picture

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

jhodgdon’s picture

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

Status: Needs review » Needs work

The last submitted patch, 16: 1926344-eliminate-search-results-twig.patch, failed testing.

The last submitted patch, 16: 1926344-eliminate-search-results-twig.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
7.33 KB
FAILED: [[SimpleTest]]: [MySQL] 59,009 pass(es), 2 fail(s), and 0 exception(s). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 22: 1926344-22.patch, failed testing.

jhodgdon’s picture

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.

jhodgdon’s picture

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.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
PASSED: [[SimpleTest]]: [MySQL] 59,139 pass(es). View
853 bytes

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

Cottser’s picture

Status: Needs review » Needs work

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?

jhodgdon’s picture

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.

Cottser’s picture

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

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
8.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1926344-30.patch. Unable to apply patch. See the log in the details link for more information. View
1.72 KB

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.

jhodgdon’s picture

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

The last submitted patch, 30: 1926344-30.patch, failed testing.

The last submitted patch, 30: 1926344-30.patch, failed testing.

jhodgdon’s picture

Looks like this one needs a reroll.

jhodgdon’s picture

FileSize
8.54 KB
FAILED: [[SimpleTest]]: [MySQL] 60,097 pass(es), 0 fail(s), and 270 exception(s). View

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

Cottser’s picture

Assigned: jhodgdon » Cottser

I would love to!

Status: Needs review » Needs work

The last submitted patch, 35: 1926344-35-reroll.patch, failed testing.

The last submitted patch, 35: 1926344-35-reroll.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
8.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,901 pass(es). View

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. :)

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs review » Needs work

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&...

Cottser’s picture

Issue tags: +Novice, +SprintWeekend2014

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

maggo’s picture

Status: Needs work » Needs review

Search results, now with classes :)

maggo’s picture

Issue tags: +D8MA
FileSize
8.71 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Patch added

Cottser’s picture

Status: Needs review » Needs work

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',
  ),
),
Cottser’s picture

+++ 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.

maggo’s picture

Status: Needs work » Needs review
FileSize
8.63 KB
PASSED: [[SimpleTest]]: [MySQL] 63,193 pass(es). View
1.04 KB

Done!

Cottser’s picture

Status: Needs review » Needs work

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.

The last submitted patch, 43: 1926344-42.patch, failed testing.

mdrummond’s picture

Assigned: Unassigned » mdrummond

I can work on this change.

mdrummond’s picture

Assigned: mdrummond » Unassigned
Status: Needs work » Needs review
FileSize
8.72 KB
PASSED: [[SimpleTest]]: [MySQL] 63,199 pass(es). View
1.64 KB

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

Cottser’s picture

Status: Needs review » Needs work
FileSize
27.97 KB
32.82 KB

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:

1926344-50-before

After:

1926344-50-after

Cottser’s picture

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',
jhodgdon’s picture

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?

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
PASSED: [[SimpleTest]]: [MySQL] 63,745 pass(es). View
3.49 KB
48.56 KB
38.53 KB

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:
Screen shot with patch in #54 with search results

No results:
Screen shot with patch in #54 with no search results

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

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!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

alexpott’s picture

Status: Fixed » Needs work

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.

Search

Search results

  1. Test page text is here

    1. Dummy title

      Dummy search snippet to display. Keywords: foo

      Conditions: Array
      (
      [search_conditions] =>
      )

Your search yielded no results.

  • Check if your spelling is correct.
  • Remove quotes around phrases to search for each word individually. bike shed will often show more results than "bike shed".
  • Consider loosening your query with OR. bike OR shed will often show more results than bike shed.
alexpott’s picture

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

The last submitted patch, 54: 1926344-54.patch, failed testing.

jhodgdon’s picture

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

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Related issues: +#1939062: Convert theme_item_list() to Twig

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.

webchick’s picture

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

Cottser’s picture

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

jhodgdon’s picture

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.

jhodgdon’s picture

Oh, cottser just did that, crosspost!

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

FileSize
8.33 KB
PASSED: [[SimpleTest]]: [MySQL] 64,217 pass(es). View

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 68: 1926344.68.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

68: 1926344.68.patch queued for re-testing.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc - random test fail.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Cottser’s picture

Thanks @alexpott!

jhodgdon’s picture

Hooray!

Status: Fixed » Closed (fixed)

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