Add templates

Your patches are both empty.

In general, I fully support this issue, though.

No, I can't see your local commits. Please read the documentation on how to create patches with Git.

Still not quite, sorry. Your patch doesn't include the template files themselves.
You can easily test this by applying the patch in reverse mode (with git apply -R foo.patch) and seeing if that leaves you with a clean module (git status returns no changes).

I'll give you a quick overview of how to create patches with Git – but usually, please consult the documentation here on Drupal.org. I think the people made a pretty good job documenting the use of Git for Drupal development.

So, if you want to create a patch for a module (no matter whether your own or another one's – as long as you don't want to commit it, but just create a patch file, there's no difference) you have two ways to do that: either just making all changes without committing, or committing all changes like normal.

In the first variant, do the following to create a patch of ALL changes in your local copy of the module:

git add -A
git diff --cached > foo.patch
git reset

To only add the changes of some files:

git add file1 file2 file3
git diff --cached > foo.patch
git reset

When you want to use commits like normal, you should create a new branch for your local work:

git checkout -b some-new-branch-name # I'd call it something like [ISSUE_NR--DESCRIPTION]
# Do some work and commit it (no matter whether with one or several commits).
git diff 7.x-1.x > foo.patch # if you want to create a patch for the 7.x-1.x branch, at least
# To get back to the clean module checkout:
git checkout 7.x-1.x

If you have worked and committed on the 7.x-1.x branch, you can still create a patch:
git diff origin/7.x-1.x
Also, you don't have to undo commits one by one, but can use, e.g.
git reset HEAD~4
to undo the last four commits (while not changing any files). To also change the files, use
git reset --hard HEAD~4

Glad I could help!
Now for a review of the actual code … I'm afraid to say that it's not really how these things should be done. I can't blame you, though, the documentation I could find of the Theme API is more like a really, really bad joke.

• The pager is still hard-coded at the page bottom, even though you can now also put it at the top.
• Likewise, the form, spellchecking suggestions, etc., should all go into a template, too, so people can completely freely re-arrange stuff. Maybe introduce a new search_api_page_results_page template for that.
• Templates shouldn't contain such complex logic. ifs with single-variable conditions, loops and render() calls are generally OK (and maybe t() calls, if it's clearer that way), but almost anything else isn't. Make the variables you pass to the template complete render arrays (in the preprocess functions) and only call render() on them, instead of theming/viewing right in the template.
• If I select „Themed as search results“ as the view mode, the results contain first a list of search results, and then all result nodes rendered in „Full“ view mode, below. If I select another view mode, nothing at all is shown. This will probably fixed by a clearer structure (i.e., following the above items), though.
• You have the <li> element in both templates, thus wrapping each search result twice (leading to invalid HTML and display problems).
• Only use variables as arguments for render(), not function calls (only variables should be passed by reference).
• The patch contains #1285116: Results per Page _GET Override, too.
• The whole thing should probably be wrapped in a singlediv with some helpful classes (e.g., search-api-page, search-api-page-{$page->machine_name}, etc.). Or, all created elements should have these classes (probably not too hard to do by using one $classes array and passing that on to the „lower“ functions.).

+++ b/search_api_page.info
@@ -4,5 +4,6 @@ description = "Create search pages using Search API indexes."
+stylesheets[all][] = search_api_page.css

This will include the CSS file in all pages, not only when viewing search results.

+++ b/search_api_page.module
@@ -58,10 +58,11 @@ function search_api_page_theme() {
-      'view_mode' => 'search_api_page_result',
+      'view_mode' => 'search_api_page_results',

Don't know why you changed this, the view mode is still named search_api_page_result.

So, in the big picture, I'd do the following:

1. In the page callback, collect form, (unthemed) results, spellcheck suggestions (maybe as a $additional variable or something like that), pager, etc. into one render array with '#theme' => 'search_api_page_results_page' (or something like that).
2. In template_preprocess_search_api_page_results_page() pass the results to theme('search_api_page_results') with the necessary additional information.
3. In template_preprocess_search_api_page_results_page() take care of rendering the individual results in the appropriate way. The search-api-page-results template (which is, by the way, what I think it should be called, not with underscores) should preferably only receive a single $results variable to output, containing a render array.

That's about it.

Please use the attached patch as the starting point for future work – it fixes some of the smaller issues, and applies cleanly to the current HEAD (with #1285116: Results per Page _GET Override already committed).

Any update on this? Currently the search page has no pager.

Another thing that could be added to this to greatly benefit themeing is to add template suggestions.

It would be desirable to be able to override the template at the index level because you might be returning greatly different results from each index.

Hey just been looking, is there a way to do this so that the blocks can be themed as well? I want to add some divs to the block...

@ajaysolutions:
Can you be a little more specific about what you mean by "the blocks"? Just to be clear.

I tested this and it seems like it's working fine... Would love to see this committed :-) ...

Hi thanks for the work,
i patched and quickly tested the template system, it seems to work for now, excepte in search-api-page-results.tpl.php L44
you forgot a "php" : "<?" -> "<?php"

I tested as well and it's working. I'm just providing a new patch which fixes the error mentioned in #17.

I tested it too and works well.
I really need this this for the Search API location module!

Just finished my theme function overrides yesterday, only now I discover this issue. Would love to see this committed!

Sorry, I was already reviewing the day before yesterday but then this happened: #735606: Let the user confirm before canceling.
So now a somewhat shortened review, sorry.

First off, many thanks to all of you for the great patch and your work!

Typos / misspellings:
In „View mode“, the m should be lower case.
You wrote several times search_api_page-results.tpl.php instead of search-api-page-results.tpl.php in comments.

Warnings I got:
Notice: Undefined index: keys in theme_search_results_list() (line 155 of …/search_api_page/search_api_page.module).
Notice: Undefined index: excerpt in template_preprocess_search_api_page_result() (line 166 of /home/thomas/Dokumente/Programmieren/PHP/Drupal/search_api_page/search_api_page.pages.inc).
Strict warning: Only variables should be passed by reference in include() (line 45 of /home/thomas/Dokumente/Programmieren/PHP/Drupal/search_api_page/search-api-page-results.tpl.php).

+++ b/search-api-page-result.tpl.php
@@ -0,0 +1,56 @@
+ * - $snippet: A small preview of the result. Does not apply to user searches.
+ * - $info: String of all the meta information ready for print. Does not apply
+ *   to user searches.
+ * - $info_split: Contains same data as $info, split into a keyed array.
+ * - $list_classes: CSS classes for this list element.
+ *
+ * Default keys within $info_split:
+ * - $info_split['user']: Author of the node linked to users profile. Depends
+ *   on permission.
+ * - $info_split['date']: Last update of the node. Short formatted.

Please remove the specific references to node / user and replace with “if available” and the like.

+++ b/search-api-page-result.tpl.php
@@ -0,0 +1,56 @@
+ * @see template_preprocess()
+ * @see template_preprocess_search_result()
+ * @see template_process()

I think this should only reference our preprocessing function, not those three.

+++ b/search-api-page-result.tpl.php
@@ -0,0 +1,56 @@
+<li<?php print $list_classes; ?>>

Why not just $classes?
Also, don't include the class="…" parte here, just include the classes themselves.

+++ b/search-api-page-results.tpl.php
@@ -0,0 +1,53 @@
+ * - $page_machine_name: Machine name of the current Search API Page.

Please pass the full page object, I don't see a reason to just pass the machine name.

+++ b/search-api-page-results.tpl.php
@@ -0,0 +1,53 @@
+  <div class="search-api-page <?php print 'search-api-page-' . $page_machine_name . ' view-mode-' . $variables['view_mode'];?>">

Use a $classes variable here, prepared in the preprocessor, and using drupal_clean_css_identifier().

+++ b/search-api-page-results.tpl.php
@@ -0,0 +1,53 @@
+      <?php if ($variables['view_mode'] == 'search_api_page_result') : // Uses child template. ?>
+        <?php print render($search_results); ?>
+      <?php else : // All other view modes (Teaser, Full content, RSS and so forth). ?>
+        <div class="search-results">
+          <?php print render(entity_view($index->item_type, $items, $variables['view_mode'])); ?>
+        </div>
+      <?php endif; ?>

For my taste, this is too much logic for a template file. Do this in the preprocessing function, please!

+++ b/search_api_page.info
@@ -4,5 +4,6 @@ description = "Create search pages using Search API indexes."
+stylesheets[all][] = search_api_page.css

This will add the CSS file to every page. Please revert to using drupal_add_css().

+++ b/search_api_page.module
@@ -69,14 +73,99 @@ function search_api_page_theme() {
+  $themes['search_performance'] = array(
+    'render element' => 'element',
+  );
+  $themes['search_results_list'] = array(

These theme function names should be properly namespaced (i.e., prefixed with search_api_page).

+++ b/search_api_page.module
@@ -69,14 +73,99 @@ function search_api_page_theme() {
+ * Returns HTML for a list of search results.
+ * Taken from theme_item_list().

There should be a newline between those two lines.

Also, more importantly: Do we really need this? Can't we just use theme_list()? What do we gain? This should also be explained in the doc comment.

It seems you just removed the search_api_page_search_results_list() function, but still reference it in your code (hook_theme() and template_preprocess_search_api_page_results()). Can this be just replaced by the normal list theme?

Also, the handling of the classes doesn't make any sense – you don't set those at all, only locally in one preprocessing function. They aren't passed on to any template.

First setting $variables['search_results'] if it might be overwritten later (if a real view mode is used) also doesn't make much sense.

There were also a number of still unfixed typos/ommission I fixed in the attached patch.

And again some notices:

Notice: Undefined index: page in template_preprocess_search_api_page_results() (line 157 of …/search_api_page/search_api_page.pages.inc).
Notice: Trying to get property of non-object in template_preprocess_search_api_page_results() (line 157 of …/search_api_page/search_api_page.pages.inc).

Next time, could you please review your patch (or at least test it) before submitting?

I also suggest we use search_api_page_results__{$page->machine_name} as the theme, so site builders can override that on a per-page basis.

Jumping into this thread late, but I've rolled an updated patch based on the feedback from #24

Really great to find this. One question, the combo patch in #27 doesn't apply to dev, only to beta2, should this be so?

I have one slight patch to submit.
I moved the form to render in the template itself. Would this be a good way to do it?

It should really be for dev.

my patch seems like a totally bad way to do this... does anyone have an idea to move the form render out of inc to tpl?

Attached is a re-roll that should apply to current dev. Please test/review so we can finally get this in!

my patch seems like a totally bad way to do this... does anyone have an idea to move the form render out of inc to tpl?

You'd have to pass the rendered form inside a variable. However, I'm not sure we should do this, the form isn't really part of the results. And users already get the option of excluding the form from the results page. Other changes can probably be as easily achieved with altering the returned render array, or just with CSS.

Re-roll for latest dev.
Could someone please take a look / test so I can commit this? I'm not a themer, so I'm not confident enough that the patch should be committed like this.

#32 works functionally, but i can't comment about theming either.

I like the templates, but i think it's overkill to force the search result to be a ul li list. Just render $search_results with no markup, I'll add whatever else i need to the templates.

Without me reading through all the code right now, does anyone know why this is using custom templates instead of drupal core's search-result.tpl.php and search-results.tpl.php.

It seems like re-inventing the wheel and adding unnecessary things to the theme registry to be making new templates.
Also, it is easier for themers if search results from all types of search modules use the same result templates.

I'm pretty sure all the search modules I have ever used use the default drupal core templates.

If no one else gets to it before me I might do a patch that uses the core templates for comparison.

I like the templates, but i think it's overkill to force the search result to be a ul li list. Just render $search_results with no markup, I'll add whatever else i need to the templates.

I'm all for clean markup but from a semantics and accessibility point of view the search results really are a list. As long as it isn't wrapped in a bunch of unnecessary divs I don't think that a single ul is excessive markup.
If everything else in drupal core and contrib was spartan by default I could understand but that is not the case.

I think it is correct to make it a list and if you don't want that then you remove what you don't want from the templates.
This means users without the skill to modify templates get nice semantic markup.

I like the templates, but i think it's overkill to force the search result to be a ul li list. Just render $search_results with no markup, I'll add whatever else i need to the templates.

I agree with rooby there, they are a list so the correct semantic list to output them is with a list.
In theory, it's even an ordered list – maybe that would be the right thing here? I just checked, core search does it the same way, so I vote for changing the list type to <ol>. Patch (with a one-character change compared to #32) attached.

@ rooby: We don't depend on core search and shouldn't, so we can't use their theme functions and templates – that's it. Also, the templates in this patch are actually based on the ones in core, see the original post. While it's certainly code duplication (though, as said, necessary one), there was no real duplication of effort.

Anyways, great to hear some positive feedback here. If there are no further objections (also against the <ol> change), I can soon finally commit this.

Oh, I think now I see what you mean, travisc: you mean the <ol> and <li> should be inside the templates instead of the search results being wrapped in an additional theme_item_list(), so you can decide not to use a list more easily.
That's of course true and also what core does, and I don't really know why it wasn't like this before. See the attached patch for a fix.
I hope now everything's fine. Thanks for complaining about this! ;)

Aaaaaand finally committed!
Phew.
Thanks again for everyone's work, especially tomogden for his continued effort!