search.module - Convert PHPTemplate templates to Twig

Looking into: DrupalCon Sydney Sprint.

patch attached

#5: 1898448-search-twig-4.patch queued for re-testing.

Thanks for this @chrisjlee!

Converting the theme() calls to render arrays (per http://drupal.org/node/1920746#render) should fix the exceptions, the 'pager' variable in preprocess would be NULL if no pager was to be rendered.

The remaining failures are legitimate and they are because the help text is missing (see search-results.html.twig). I added that as a temporary @todo but that will need to be fixed in this issue.

The only other changes I made were removing one extra newline, and removing spaces around the 't' filter.

Discussed briefly with @steveoliver, let's put the help text in the preprocess for now and revisit later. This should pass testing.

No problem! Awesome it passed.

Thanks for your help. This helps me figure out the patches.

Thanks guys, this is a great start! A few comments:

1) In the .html.twig template comment docs we need to remove any mention of variable type, since front-end devs won't need to know what an array or string is anymore. It might be worth reading our coding standards to get up to speed on all these little nuances :)

 * - title_prefix (array): An array containing additional output populated by
 *   modules, intended to be displayed in front of the main title tag that
 *   appears in the template.
 * - title_suffix (array): An array containing additional output populated by
 *   modules, intended to be displayed after the main title tag that appears in
 *   the template.

should become

 * - title_prefix: Additional output populated by modules, intended to be
 *   displayed in front of the main title tag that appears in the template.
 * - title_suffix: Additional output populated by modules, intended to be
 *   displayed after the main title tag that appears in the template.

and

 * - title_attributes: Array of html attributes for the title. It is
 *   flattened into a string within the variable title_attributes.
 * - content_attributes: Array of html attributes for the content. It is
 *   flattened into a string within the variable content_attributes.

should become

 * - title_attributes: HTML attributes for the title.
 * - content_attributes: HTML attributes for the content.

2) We decided to remove all instances of "is defined" from the checks, and will also only define variables as needed. That means that

  {% if (info_split.comment is defined) %}

should become

  {% if (info_split.comment) %}

We changed our mind about that a few times, so it's not uncommon to find lots of instances of "is defined" in the sandbox, but we need to remove them in the core patches.

3) All instances of render() and similar need to be removed from templates. This means that

  {{ render_var(title_prefix) }}

should become

{{ title_prefix }}

If we can make these small changes, I think this patch looks great. I'm going to create a follow-up issue about combining these two templates into one though, since we are splitting the OL and the LI tags by doing it this way, and that goes against our new principles for the Drupal 8 theme layer.

edit: Follow-up issue created at #1926344: Consolidate search-result.html.twig and search-results.html.twig

Re-rolled

This is much better :)

I think we still need to get rid of the docs that mention that info_split is a keyed array.

 * - info_split: Contains same data as info, split into a keyed array.

Long term, we should aim to get rid of info_split entirely. We can nest each one of the sub-items within info directly to make the info variable drillable, so printing {{ info }} will work exactly as it does now, but printing {{ info.type ]] should print only the type, as we do currently by printing {{ info_split.type }}. But I think that is too much for this first patch :)

For now, how about we just move the mention of info_split lower in the docs right before we explain what it contains? Something more like...

 * - info_split: Contains the same data as info, but split into separate parts.
 *   - info_split.type: Node type (or item type supplied by module).
 *   - info_split.user: Author of the node (can be linked to users profile
 *     depending on permission).
 *   - info_split.date: Last update of the node. Short formatted.
 *   - info_split.comment: Number of comments output as "% comments", %
 *     being the count (depends on comment.module).
 * @todo: the info variable needs to be made drillable and each of these sub
 *   items should instead be within info and renamed info.foo, info.bar, etc.

The @todo in here will remind us to clean this up later as we get to #7, Cleanup of previous preprocess functions.

Also, I missed this in the last review, but the docs still say that these templates contain definition lists which is not the case anymore, so we should correct that as well.

in search-results.html.twig:

 * This template collects each invocation of theme_search_result(). This and
 * the child template are dependent to one another sharing the markup for
 * definition lists.

should become

 * This template collects each invocation of theme_search_result(). This and
 * the child template are dependent to one another sharing the markup for
 * ordered lists.

And in search-result.html.twig:

 * This template renders a single search result and is collected into
 * search-results.twig. This and the parent template are
 * dependent to one another sharing the markup for definition lists.

should become

 * This template renders a single search result and is collected into
 * search-results.twig. This and the parent template are
 * dependent to one another sharing the markup for ordered lists.

Jennifer:

Thanks for the helpful comments and detailed changes.

Thanks @chrisjlee, the latest patch included changes to UpdateCoreUnitTest.php, quickly rerolled (edited) to remove those hunks.

Minor nitpick (not changed in this patch): the @todo for the info variable is not formatted correctly, see http://drupal.org/node/1354#todo. No colon, and the first word after the @todo should be capitalized.

Cottser:

Good catch! made those changes

Whoops sorry forgot to hit attach.

More teeny tiny nits :)

In search-result.html.twig
We changed the names of our templates from .twig to .html.twig, so we need to update the docs to reflect that.

 * This template renders a single search result and is collected into
 * search-results.twig. This and the parent template are
...

In search-results.html.twig
We print {{ pager }} in the template, but it's not listed in the Available variables. We should add it.

in search.pages.inc we need to clean up the preprocess function docs, now that we have standards :)

/**
 * Prepares variables for search results templates.
 *
 * Default template: search-results.html.twig.
 *
 * @param (array) $variables
 *   An array with the following elements:
 *   - results: Search results array.
 *   - module: Module the search results came from (module implementing
 *     hook_search_info()).
 */

and it also looks like the list of elements within $variables was incorrect for template_preprocess_search_result :( let's see if this is a sufficient improvement.

/**
 * Prepares variables for individual search result templates.
 *
 * Default template: search-result.html.twig
 *
 * @param (array) $variables
 *   An array with the following elements:
 *   - result: Individual search result.
 *   - module: Module the search results came from (module implementing
 *     hook_search_info()).
 *   - title_prefix: Additional output populated by modules, intended to be
 *     displayed in front of the main title tag that appears in the template.
 *   - title_suffix: Additional output populated by modules, intended to be
 *     displayed after the main title tag that appears in the template.
 *   - title_attributes: HTML attributes for the title.
 *   - content_attributes: HTML attributes for the content.
 */

So close guys! :)

re-rolled.

Thanks @joelpittet!

The standards aren't quite official yet, but this won't change: the array data type in the @param should be formatted like @param array $variables. No surrounding parentheses for the array per http://drupal.org/node/1354#param.

like this?

Yup, thanks @chrisjlee!

#23: 1898448-23.patch queued for re-testing.

Thanks Cottser, that one got me twice!
Re-uploading patch from #23 since test bot seems to not want to re-test patches? (it's not actually in the queue)

@jenlampton - #23 had some random test failures but it did come back green after I re-queued this morning. No biggie :)

Tests passing, and... looks fabulous! :) :)

This is looking good! Found some more minor docs tweaks, minor enough that this can probably be left as RTBC.

+++ b/core/modules/search/templates/search-result.html.twigundefined
@@ -0,0 +1,77 @@
+ * - title_prefix: Additional output populated by modules, intended to be
+ *   displayed in front of the main title tag that
+ *   appears in the template.

This comment should be re-wrapped at 80 characters, the second line ends too early.

+++ b/core/modules/search/templates/search-result.html.twigundefined
@@ -0,0 +1,77 @@
+ * @see template_process()

Should this be removed?

+++ b/core/modules/search/templates/search-results.html.twigundefined
@@ -0,0 +1,34 @@
+ * - search_results: All results as it is rendered through search-result.tpl.php
+ * - module: The machine-readable name of the module (tab) being searched, such
+ *   as 'node' or 'user'

These comments needs to end in periods, and in one case (search_results) be wrapped at 80 characters as a result.

@Cottser Like that? did those changes except this one:

+++ b/core/modules/search/templates/search-result.html.twigundefined
@@ -0,0 +1,77 @@
+ * @see template_process()

Because i don't really know the answer to that question.

Sorry realized i just made a small typo.

try this patch instead.

Thanks @chrisjlee! Everything looks good, except one thing…

+++ b/core/modules/search/templates/search-results.html.twigundefined
@@ -11,7 +11,8 @@
  * - module: The machine-readable name of the module (tab) being searched, such
  *   as 'node' or 'user'

This comment still needs a period at the end, after 'user'.

Oh man, so close I can taste it.

Hmm... Let's leave this for now:

 * @see template_process()

I have been removing lines like these since we will *eventually* be removing the whole process layer, but perhaps it's premature to remove them in these initial patches, since this function still exists. We can roll the removal of all these @see lines into the patch that removes that function. That should be fun :) Dibbs!

@chrisjlee do you want to do the honors of adding the final period as per #33?

Yes I would love to. Will do that tomorrow!

Added a period. Hooray! I've never been so excited about adding a period!

Changed status

Aw yeah :)

added to test section

Added a commit message to the issue summary based on git blame and sandbox issues. Please edit if I missed anyone :)

#36: 1898448-36.patch queued for re-testing.

#36: 1898448-36.patch queued for re-testing.

Committed and pushed to 8.x. Thanks!