Search results are themed in search_data(). It should instead return a renderable array.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Status: Active » Needs review
FileSize
6.6 KB

Here's a patch:
- Makes search_data() return a renderable array.
- Correspondingly, search_view() needed a slight change so that it would expect a renderable array from search_data().
- I wanted to make sure this change didn't break the hook_search_page() API, since it affected code near that. There wasn't a test for invoking hook_search_page(), so I added a test.

Kitten injury warning! This patch also fixes the following possibly off-topic problems (but I think they're related enough to fix with this issue):
- The documentation for hook_search_page() had a small typo (referenced hook_search() which no longer exists), so I fixed that.
- The example function body for hook_search_page() didn't work at all as written, so I fixed that (I used it for the test, which is how I realized it was unworkable).
- There wasn't a mention of hook_search_page() in hook_search_info() doc, so I added that (and also added a mention for the other search hook, hook_search_access() while I was at it).
- Fixed up a couple of comment/doc oversights in search.test and its supporting test module.

I can take out the kitten injury parts if you want and put them in separate issues, but they are somewhat related...

jhodgdon’s picture

FileSize
6.57 KB

Whoops. Fix newline warning in previous patch...

pwolanin’s picture

Status: Needs review » Needs work

Looks basically good to me - I noticed this during the recent changes, but didn't get around to changing it.

However, it now has the wrong default result:

  $results = '';
pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.54 KB

fixes the default results.

pwolanin’s picture

Status: Needs review » Needs work

To be consistent, we should redefine the return value for hook_search_page

Also, having #type in the renderable array may cause issue s- let's make the conversion here is #module.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.28 KB

ok, with fixes for that and making it all consistent.

jhodgdon’s picture

Issue tags: +API change

I noticed one thing. template_preprocess_search_result() does this:

+  if (!empty($result['module'])) {
+    $info['module'] = check_plain($result['module']);

The search_results function needs to do the same check_plain I think?

  if (!empty($variables['module'])) {
    $variables['module'] = check_plain($variables['module']);
  }

Other than that, I think this is good to go.

API CHANGE NOTES:
- search_data() return value has changed to be a renderable array rather than rendered HTML
- hook_search_page() return value has changed to be a renderable array rather than rendered HTML
- theme('search_result') and theme('search_results') and their templates/processing functions now use a 'module' argument/variable in place of 'type'.

jhodgdon’s picture

FileSize
666 bytes

Here's a new patch with the above check_plain added.

jhodgdon’s picture

FileSize
10.88 KB

Whoops.
Here's the right patch. Doh.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

that's maybe overkill since $variables['module'] is pretty well going to be safe, but doesn't hurt anything.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This looks very ready. Couple of small things:

+++ modules/search/search-results.tpl.php	17 Aug 2010 16:44:11 -0000
@@ -15,7 +15,7 @@
+ * - $module: The module searching, e.g., "node" or "user".

This is a bit weird. If one has Apache Solr installed, isn't Apache Solr the module searching?

+++ modules/search/tests/search_extra_type.module	17 Aug 2010 16:44:11 -0000
@@ -45,3 +48,23 @@
+  $output['suffix']['#markup'] =  '</ol>' . theme('pager');

Two spaces?

Powered by Dreditor.

pwolanin’s picture

The $module name is the machine-readable name, so "node", "user", or "apachesolr". In other words, the module that implements hook_search_*

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.26 KB

Here's a new patch addressing (I hope) #11. Taking the liberty of setting to RTBC, since all I did was change the wording of the search-result/search-results $module variable, and remove the extra spaces in search_extra.module

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture

Status: Closed (fixed) » Needs work
Issue tags: +Needs documentation updates

I think we need to add this to the theme update 6/7 guide, and possibly the module update 6/7 guide. See #7 for summary.

jhodgdon’s picture

Status: Needs work » Fixed

This is now documented in the theme and module update guides.

Status: Fixed » Closed (fixed)

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

Tom-E’s picture

Hi,

Recently bumped into an issue with this commit.

There's definitely an inconsistency between search-result.tpl.php and the current code, and I'd like to raise a concern about the change from 'type' to 'module'.

At the very least that template file needs to be corrected so it doesn't mention 'type' anymore... But,

Previously search-result.tpl.php could easily use $info_split['type'] to modify how individual Content-Types are themed in search results.

I believe the change was made because also, previously $info_split['type'] outputted the Human formatted name, not the internal content-type name -- which is of course weird.

re: template_preprocess_search_result() - Could this be re-addressed?
'module' - doesn't always get added, for example when you are using default search module and want custom formatting for a content-type via overridden theming file.
'type' - was previously human readable version, kind of ugly.

could we maybe go with
'module' - stays as is, a new addition to D7
'type' - as it was in D6, or make it the internal name
?

jhodgdon’s picture

Status: Closed (fixed) » Needs work

If you want to bring up a concern, you need to reopen the issue. :)

jhodgdon’s picture

Status: Needs work » Fixed

Actually, this seems like a separate issue, and unfortunately, it is probably too late to do this for Drupal 7, since we try to avoid more API changes between 7.x and 7.y versions. So if you would like this to be done, please file a separate issue as a feature request for Drupal 8.

aspilicious’s picture

Status: Fixed » Needs work
+++ modules/search/search.api.php	18 Aug 2010 14:00:09 -0000
@@ -262,17 +264,20 @@
+    $output[] = array(
+      '#theme' => 'search_result', ¶
+      '#result' => $entry, ¶
+      '#module' => 'my_module_name',

trailing whitespaces

Powered by Dreditor.

jhodgdon’s picture

removing tag.

Anonymous’s picture

Status: Needs work » Fixed

Wait, isn't this fixed? I can't see any real difference between when it was marked fixed in #17 and now.

Status: Fixed » Closed (fixed)
Issue tags: -API change

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