Posted by hutch on June 25, 2006 at 1:29pm
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | search.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | rocket_nova |
| Status: | needs review |
Issue Summary
The attached patch is on
search.module,v 1.176.2.2 2006/05/07 17:51:24 killes
It adds some checkboxes to the search settings page to control the display of Date, Author, Node type and Extras in the search results page.
HTH
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| search.module_display_results_settings.patch.txt | 5.34 KB | Ignored | None | None |
Comments
#1
search keywords module is not search.module.
#2
we only accept new features against drupal CVS HEAD (the upcoming 4.8)
#3
a lot of changes were made between drupal HEAD and 4.7
--> i guess it wont apply
#4
reapplied
#5
Not sure how important this is. One some sites it is probably useful to hide the date and author. And on static sites, the content type might be confusing. We'll want to come up with a better description for "extras". Extra's is not exactly helpful for a user. If you mean 'result snippets', write 'result snippets' (or something equivalent). Otherwise I'm cool with this patch.
#6
Search results are already themable, do we really need explicit toggles?
The texts need work though. A description à la "Show the date in the results" is a bit redundant if you do it for every checkbox. How about naming the fieldset "Show in search results:" and removing the descriptions? "Node type" should be "Content type". And "extras" can be better too... perhaps "module-provided fields"?
Shouldn't we use #type => checkboxes for this as well?
#7
I agree this is a non-important feature, but for some people this kind of tweaks can be quite usefull... Anyway, I fixed the texts as you suggested, but I also moved the form to the theme settings page following the example of post information settings.
#8
Tweaking your site using settings is always easier and accessible to non-developers / non-designers. It's a useful feature IMO, and the code looks fast and elegant.
At least, the first patch looked elegant. The second patch seems rather complex ... not sure this ought to be a theme setting.
#9
It's back on the search settings page...
#10
...and ready for review...
#11
This patch works well for me. I like it.
However, I'd like the search module to also care about the theme settings (as in
theme_get_setting('toggle_node_info_' . $node->type)). If a node type requires not to display post information, I think the search results should not show such information. Maybe this is a separate issue though.#12
A bit late to try to revive this issue, but who knows? ;-)
#13
#14
Re-rolled the patch (not taking into account my comment in #11).
#15
Marking http://drupal.org/node/107506 as a duplicate.
Furthermore, per content type setting from theme configuration could be imposed, as pointed on above link?
#16
Yes, I'll make a new patch to take the theme setting into account. I also need to rename the new 'search_result_node_info' variable; it is misnamed because this setting does not apply only to nodes. I should have kept the same name as the previous patch. ;-)
#17
This new patch takes theme settings into account and uses a proper variable name.
#18
Removed an unwanted whitespace.
#19
If anyone is interested in getting this frequently requested feature in Drupal 6, please review and test the patch before the code freeze (June 1st). It's a small and easy patch! :-)
#20
<?php'#description' => t('Select which additional information, besides title and snippet, should be displayed for each search result. This setting will have no effect on content types whose post information is never displayed (as per your !theme).', array('!theme' => l(t('theme settings'), 'admin/build/themes/settings/global'))),
?>
This usage of t()/l() is incorrect.
#21
Fixed!
#22
Umm, use this patch instead.
#23
Targetting D7, although the patch still works on D6 at the moment.
#24
Would consider the 'post information' displayed on search result pages as a bug. It's really odd, because you don't want to show who created a content page; and if you have search module enabled, anyone can do a query and find the actual author and date of publishing. An example: Handbook pages on drupal.org.
If we can at least create a patch against search.module for Drupal 6.x-dev for respecting the "Display post information" settings in theme configuration, as a part of bug fix actually.
#25
Good point! Here's the patch. ;-)
#26
The patch is to the point for the actual bug fix!! If there's anything else demanded(except a comment?), that would be a feature request most probably. The patch attempts the minimal and best so far:
<?phpif ($type != 'node' || theme_get_setting('toggle_node_info_'. $item['node']->type)) {
// do the post info. stuff
}
?>
Hopefully, this is getting in soon. :)
#27
Brilliant idea to apply the node toggles here, it is in fact a bugfix this way :) A line of comment would be good though.
#28
Added a comment.
#29
If the patch still applies, anyone going to commit? :)
#30
It still applies. ;-)
#31
Not any more it doesn't.
#32
I looked at a few of the patches in the comments above, and the latest patch now looks incomplete. Fixing the latest patch to use search-result.tpl.php is trivial (patch attached), but someone else should add the settings form back. Was this dropped intentionally?
#33
As of today, the settings form is there (in admin/build/themes/settings). However, the above patch did not work because
$info_split['type']is a "search type", and the patch seemed to expect a node type instead.It could be nice to keep the template simple, so I'm proposing this new (and tested) patch that does not affect the tpl file.
#34
tested:
1. tried mixed search, result contains node types with and without post info
2. tried to write print $info_split['user'] into tpl file, to get php error
works without any problem and code looks good
changed patch to apply from drupal root
#35
no, sorry it does show php notice for test #2
#36
search-result.tpl.php
#37
this is the minimal set of keys required.
#38
#39
@Pasqualle: Thanks for the testing! However, the conditions you've added in your last patch are redundant, and I don't think they're of any use. As you have quoted above regarding $info_split:
Checking for existence means using
isset()in the tpl file if it needs to use a value from $info_split. The default search-result.tpl.php as it stands now does not use $info_split, and there are no PHP warnings or notices.My recommendation would be to go with the patch at #34.
#40
read further please
As I read it, the whole description, there are keys like comment and upload and other possible module generated keys which are not guaranteed to be set, but these 3 are always set.
Of course if you examine the code closely, it was not guaranteed even before the patch, but that is a bug, as I understand.
It is basic requirement for css only themes to set all variables used, otherwise you will get php notices.
If you want to go with patch #34, then comment in search-result.tpl.php must be modified also.
#41
Ah, yes... Good catch!
Except for 'type', 'user' and 'date', any value in $info_split already has to be checked for existence, so it seems more consistent to do the same for 'type', 'user' and 'date' as well. At least that's what the patch at #34 implied. This new one changes the comments in search-result.tpl.php to reflect this logic.
#42
#43
still applies.
#44
LIVE FROM THE MINNESOTA SEARCH SPRINT: This patch still applies and works.
#45
LIVE FROM THE MINNESOTA SEARCH SPRINT
Changing the title to better reflect the issue.
Currently, search results reveal authorship and other 'post' information, even if a site admin has elected to hide this information in the theme settings. This could be construed as an information leak.
This patch fixes that issue, by checking whether the theme allows post information to be displayed before populating the search results.
#46
LIVE FROM THE MINNESOTA SEARCH SPRINT
Reviewed code; variable setting code in template_preprocess_search_result() is now executed conditionally for non 'node' type results or node types for which 'toggle_node_info_' is set.
Tested as follows:
* Before the patch is applied, all post information is displayed for all node types in search results
* With patch applied, enabled 'display post information' for a given set of types. Typed a search term which displays results encompassing all node types. Only the types for which 'display post information' is enabled now have post information displayed in search results. Tried this for several different combinations of types.
#47
puregin: can you try writing a simpletest for this? It shouldn't be too hard.
#48
puregin: can you try writing a simpletest for this? It shouldn't be too hard.
#49
#50
Hi,
This weekend PHP Quebec organized a Code Fest; I used this occasion to write a simpletest to test the presence of node info in the test result. Here is the new patch for the module.
#51
#52
Patch need to be rolled.
#53
Rerolled and tested. Updated some of the code comments. I tested in a default installation where articles had "display post information" enabled and pages did not. Search results reflected this detail.
One concern. In the simpletest, the search results are tested like this:
<?php// Parse result and look for <p class="search-info">...</p>; there should not be any.
$this->parse();
$this->assertFalse($this->elements->xpath('//p[@class="search-info"]'));
?>
This assumes that the only information in search-info is going to be the author/date post information that is controlled by the theme settings. What if another module comes along and adds to search results? I think the tests would fail.
On the other hand, the search-info HTML is so unstructured that there is no other way to test this, and the test works perfectly fine for the Drupal that we have now.
If we're all happy with the very small chance that this test may need to be rewritten in the future if another core module decides to start embellishing the search-info array, then this looks like a good patch to me.
#54
Code looks good; need to test.
#55
The last submitted patch failed testing.
#56
Could we have a bit of assistance with this one?
We just discovered that even though author information is not displayed on nodes, when you search for content THEN it shows up. Very UN-institutive in my mind.
We need this for D5 sites.
Thanks
#57
@mdlueck: I think in D5 it is possible to override the theme function of the search result form.
#58
@Pasqualle: Thanks for the reminder. Implemented!
#59
Re-test of search_results.patch from comment #53 was requested by sun.
#60
The last submitted patch, search_results.patch, failed testing.
#61
#53: search_results.patch queued for re-testing.
#62
The last submitted patch, search_results.patch, failed testing.
#63
Looks like we need a reroll of this patch.
#64
Any progress with the patch?
#65
Since you are obviously interested in the outcome of this issue... If you have some time, more productive than asking whether someone else has made any progress on this issue would be to reroll the patch above in #53 so that it will apply, attach it here, and set the status back to "needs review".
#66
Here's a reroll of this patch.
#67
Assume the test bot agrees, I think we should get this into D7, and then backport to D6.
#68
The last submitted patch, 70722-reroll.patch, failed testing.
#69
These failures are in Search module test cases, so they look real. Will need to investigate...
#70
This whole patch is not going to work, as it is. Issues with that patch:
- Tests fail because it is too aggressive - it's turning off *all* extra information, not just the node author and date.
- The current way to look up whether post information is displayed for a certain content type is to do variable_get('node_submitted_' . $node->type, TRUE), not what is done in this patch.
- The node module's node_search_execute() doesn't actually give us the internal type name of the node type, but the displayed type name. hook_search_execute() is only supposed to give us displayable information anyway, so that is actually reasonable. so we don't have the type's internal name during template_preprocess_search_result(), so we can't really do the variable_get that we would want to do there. One alternative would be to do it in node_search_execute(), but that would take away the theme's ability to display the information if it wanted to.
- The test should actually test setting display/not by submitting the node type edit form ('admin/structure/types/manage/page') rather than using a variable_set directly.
Taking all of this into account, I think this is a can of worms. A themer can easily display whatever they want to in search results, and we should leave it at that.
I am going to send this off to Drupal 8 for consideration of whether we should make a bunch of checkboxes to control what is displayed by default on search results, on a per-node-type basis I would guess, although my inclination is to mark it "Won't Fix".
#71
#18: search_result_node_info_1.patch queued for re-testing.
#72
Trying to address jhodgdon's concerns in #70. This issue is related to #421586: Creator nevertheless displayed in RSS feeds if disabled for privacy reasons. I think that a similar approach could be taken here where the post settings "Display author and date information." controls whether the author and date information is displayed for a content type by default. I've attached a patch to that effect here.
#73
Re-roll patch from #72 after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.
#74