Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Search results are themed in search_data(). It should instead return a renderable array.
Comment | File | Size | Author |
---|---|---|---|
#13 | 839524-13.patch | 11.26 KB | jhodgdon |
#9 | 839524-8.patch | 10.88 KB | jhodgdon |
#8 | 884932-9.patch | 666 bytes | jhodgdon |
#6 | 839524-search-theme-6.patch | 10.28 KB | pwolanin |
#4 | 839524-search-theme-4.patch | 6.54 KB | pwolanin |
Comments
Comment #1
jhodgdonHere'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...
Comment #2
jhodgdonWhoops. Fix newline warning in previous patch...
Comment #3
pwolanin CreditAttribution: pwolanin commentedLooks 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:
Comment #4
pwolanin CreditAttribution: pwolanin commentedfixes the default results.
Comment #5
pwolanin CreditAttribution: pwolanin commentedTo 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.
Comment #6
pwolanin CreditAttribution: pwolanin commentedok, with fixes for that and making it all consistent.
Comment #7
jhodgdonI noticed one thing. template_preprocess_search_result() does this:
The search_results function needs to do the same check_plain I think?
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'.
Comment #8
jhodgdonHere's a new patch with the above check_plain added.
Comment #9
jhodgdonWhoops.
Here's the right patch. Doh.
Comment #10
pwolanin CreditAttribution: pwolanin commentedthat's maybe overkill since $variables['module'] is pretty well going to be safe, but doesn't hurt anything.
Comment #11
Dries CreditAttribution: Dries commentedThis looks very ready. Couple of small things:
This is a bit weird. If one has Apache Solr installed, isn't Apache Solr the module searching?
Two spaces?
Powered by Dreditor.
Comment #12
pwolanin CreditAttribution: pwolanin commentedThe $module name is the machine-readable name, so "node", "user", or "apachesolr". In other words, the module that implements hook_search_*
Comment #13
jhodgdonHere'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
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #16
jhodgdonI 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.
Comment #17
jhodgdonThis is now documented in the theme and module update guides.
Comment #19
Tom-E CreditAttribution: Tom-E commentedHi,
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
?
Comment #20
jhodgdonIf you want to bring up a concern, you need to reopen the issue. :)
Comment #21
jhodgdonActually, 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.
Comment #22
aspilicious CreditAttribution: aspilicious commentedtrailing whitespaces
Powered by Dreditor.
Comment #23
jhodgdonremoving tag.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedWait, isn't this fixed? I can't see any real difference between when it was marked fixed in #17 and now.