Updated: Comment #77 (comments before #70 are too old to be very useful)
Problem/Motivation
Node content types have a setting "Display author and date information." that allows a site builder to show/hide this information on a per-content type basis. This is not respected in search results.
Proposed resolution
Check this setting from the template.
Remaining tasks
Decide if it's actually possible / practical to fix this issue (see #70)
Write and review the patch
User interface changes
No admin changes. Search results will no longer show this information that should not be shown.
API changes
None
Beta phase evaluation
Issue category | Bug because the UI says "don't display the author/date" but Search is violating this, so it violates expectations. |
---|---|
Issue priority | Just a normal bug. |
Prioritized changes | The main goal of this issue is to fix a usability bug, which is a prioritized change (both Usability and Bugs are prioritized). As a side effect, we also updated uses of drupal_render() (which is deprecated) to use the Renderer service instead, so that is also a Good Thing. |
Disruption | Not disruptive, no API change. |
Comment | File | Size | Author |
---|---|---|---|
#112 | 70722-112.patch | 2.85 KB | aerozeppelin |
#99 | interdiff-70722-96-99.txt | 2.56 KB | bircher |
#99 | 70722-99.patch | 8.58 KB | bircher |
#96 | interdiff-70722-94-96.txt | 1.09 KB | bircher |
#96 | 70722-96.patch | 8.33 KB | bircher |
Comments
Comment #1
sugree CreditAttribution: sugree commentedsearch keywords module is not search.module.
Comment #2
Tobias Maier CreditAttribution: Tobias Maier commentedwe only accept new features against drupal CVS HEAD (the upcoming 4.8)
Comment #3
Tobias Maier CreditAttribution: Tobias Maier commenteda lot of changes were made between drupal HEAD and 4.7
--> i guess it wont apply
Comment #4
paranojik CreditAttribution: paranojik commentedreapplied
Comment #5
Dries CreditAttribution: Dries commentedNot 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.
Comment #6
Steven CreditAttribution: Steven commentedSearch 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?
Comment #7
paranojik CreditAttribution: paranojik commentedI 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.
Comment #8
Dries CreditAttribution: Dries commentedTweaking 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.
Comment #9
paranojik CreditAttribution: paranojik commentedIt's back on the search settings page...
Comment #10
paranojik CreditAttribution: paranojik commented...and ready for review...
Comment #11
David Lesieur CreditAttribution: David Lesieur commentedThis 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.Comment #12
David Lesieur CreditAttribution: David Lesieur commentedA bit late to try to revive this issue, but who knows? ;-)
Comment #13
David Lesieur CreditAttribution: David Lesieur commentedComment #14
David Lesieur CreditAttribution: David Lesieur commentedRe-rolled the patch (not taking into account my comment in #11).
Comment #15
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedMarking http://drupal.org/node/107506 as a duplicate.
Furthermore, per content type setting from theme configuration could be imposed, as pointed on above link?
Comment #16
David Lesieur CreditAttribution: David Lesieur commentedYes, 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. ;-)
Comment #17
David Lesieur CreditAttribution: David Lesieur commentedThis new patch takes theme settings into account and uses a proper variable name.
Comment #18
David Lesieur CreditAttribution: David Lesieur commentedRemoved an unwanted whitespace.
Comment #19
David Lesieur CreditAttribution: David Lesieur commentedIf 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! :-)
Comment #20
Steven CreditAttribution: Steven commentedThis usage of t()/l() is incorrect.
Comment #21
David Lesieur CreditAttribution: David Lesieur commentedFixed!
Comment #22
David Lesieur CreditAttribution: David Lesieur commentedUmm, use this patch instead.
Comment #23
David Lesieur CreditAttribution: David Lesieur commentedTargetting D7, although the patch still works on D6 at the moment.
Comment #24
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedWould 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.
Comment #25
David Lesieur CreditAttribution: David Lesieur commentedGood point! Here's the patch. ;-)
Comment #26
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedThe 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:
Hopefully, this is getting in soon. :)
Comment #27
Gábor HojtsyBrilliant idea to apply the node toggles here, it is in fact a bugfix this way :) A line of comment would be good though.
Comment #28
David Lesieur CreditAttribution: David Lesieur commentedAdded a comment.
Comment #29
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedIf the patch still applies, anyone going to commit? :)
Comment #30
David Lesieur CreditAttribution: David Lesieur commentedIt still applies. ;-)
Comment #31
catchNot any more it doesn't.
Comment #32
douggreen CreditAttribution: douggreen commentedI 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?
Comment #33
David Lesieur CreditAttribution: David Lesieur commentedAs 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.
Comment #34
Pasqualletested:
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
Comment #35
Pasqualleno, sorry it does show php notice for test #2
Comment #36
Pasquallesearch-result.tpl.php
Comment #37
Pasquallethis is the minimal set of keys required.
Comment #38
PasqualleComment #39
David Lesieur CreditAttribution: David Lesieur commented@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.
Comment #40
Pasqualleread 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.
Comment #41
David Lesieur CreditAttribution: David Lesieur commentedAh, 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.
Comment #42
robertDouglass CreditAttribution: robertDouglass commentedComment #43
robertDouglass CreditAttribution: robertDouglass commentedstill applies.
Comment #44
David Lesieur CreditAttribution: David Lesieur commentedLIVE FROM THE MINNESOTA SEARCH SPRINT: This patch still applies and works.
Comment #45
puregin CreditAttribution: puregin commentedLIVE 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.
Comment #46
puregin CreditAttribution: puregin commentedLIVE 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.
Comment #47
Dries CreditAttribution: Dries commentedpuregin: can you try writing a simpletest for this? It shouldn't be too hard.
Comment #48
Dries CreditAttribution: Dries commentedpuregin: can you try writing a simpletest for this? It shouldn't be too hard.
Comment #49
douggreen CreditAttribution: douggreen commentedComment #50
pfournier CreditAttribution: pfournier commentedHi,
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.
Comment #51
pfournier CreditAttribution: pfournier commentedComment #52
lilou CreditAttribution: lilou commentedPatch need to be rolled.
Comment #53
robertDouglass CreditAttribution: robertDouglass commentedRerolled 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:
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.
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedCode looks good; need to test.
Comment #56
mdlueck CreditAttribution: mdlueck commentedCould 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
Comment #57
Pasqualle@mdlueck: I think in D5 it is possible to override the theme function of the search result form.
Comment #58
mdlueck CreditAttribution: mdlueck commented@Pasqualle: Thanks for the reminder. Implemented!
Comment #61
jhodgdon#53: search_results.patch queued for re-testing.
Comment #63
jhodgdonLooks like we need a reroll of this patch.
Comment #64
goofrider CreditAttribution: goofrider commentedAny progress with the patch?
Comment #65
jhodgdonSince 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".
Comment #66
jhodgdonHere's a reroll of this patch.
Comment #67
jhodgdonAssume the test bot agrees, I think we should get this into D7, and then backport to D6.
Comment #69
jhodgdonThese failures are in Search module test cases, so they look real. Will need to investigate...
Comment #70
jhodgdonThis 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".
Comment #71
bakr CreditAttribution: bakr commented#18: search_result_node_info_1.patch queued for re-testing.
Comment #72
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedTrying to address jhodgdon's concerns in #70. This issue is related to #421586: Author 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.
Comment #73
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedRe-roll patch from #72 after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.
Comment #74
drupal_was_my_past CreditAttribution: drupal_was_my_past commentedComment #75
kscheirer#73: search-post-info-with-test-70722-73.patch queued for re-testing.
+1 for "won't fix", I think there's a search result template that makes this pretty easy to customize anyway.
Comment #77
ianthomas_ukRewrote the title and summary for the new millennium (ok, decade).
-1 for won't fix. We provide a setting, therefore we should respect it. At the very least we should change the text for this option so people don't have the expectation that it would hide this information from search results.
Comment #78
ianthomas_ukComment #79
hass CreditAttribution: hass commentedNo, usernames must be removed. Otherwise the setting is useless!
Comment #80
jhodgdonOK then. I guess I agree. So we need to go back to the patch in #73 and redo it, with these changes, because a lot in Core has changed since that patch:
a) The test lines should be added to a particular test class... In the existing patch they were added to the AdvancedSearch test, which is probably OK, or they could be added to SearchExactTest.
b) Obviously variable_get/set() isn't the way to get/set the config for node display any more. Instead it would be something like:
and you may need to substitute something local for \Drupal::config() there.
Does anyone care to make a new patch? It isn't really too complicated...
Comment #81
jhodgdonShould be a good Novice issue?
Comment #82
petar.gnjidic CreditAttribution: petar.gnjidic commentedI'll take this on DDD2015
Comment #83
bircherSorry @petar.gnjidic I should have claimed it when starting to work on it.
Since you are now familiar with the issue you will have an opinion to review it though and we can fix it soon.
Comment #84
petar.gnjidic CreditAttribution: petar.gnjidic commentedHere is the partial patch.
Comment #85
jhodgdonHey bircher, I was working with Petar on his patch... yours does pretty much the same thing.
The problem is that when we looked at this in an actual site in Drupal 8, although this approach does remove the post/date information at the bottom of the search result, you can still see it in the search snippet above.
I am not sure how to remove the information in the search snippet. Petar and I tried but we did not succeed yet.
So... still needs work I think.
Comment #87
jhodgdonLooking at these two patches... I am unsure about whether it is better to do this in the preprocess function (as in the patch in #83) or in NodeSearch itself (as in the patch in #84)... thoughts?
Also as I noted in a previous comment, when we were working on the patch that is in #84, we noticed that even with this patch... let me give an example.
If you install with Standard profile and make a page whose body is "This is a test", and then search for the word "test", you will see something like this in your search result snippet:
So the patch doesn't remove the user name and date from the snippet, only from the meta-data line.
I am not sure how/why the test in #83 passed, given this, because I am pretty sure this wouldn't be fixed by the patch in #83 either?
Comment #88
jhodgdonJust to make sure, I tested the patch in #83 manually on simplytest.me.
a) Standard install
b) Create a Page with title/body "Page test", and an article with title/body "Article test".
c) Run cron
d) Search for "test"
Got the following screen shot. Note that the "admin" and date do not show in the meta-data row for the page and do for the article, which is correct. But I still see "admin" and date in the snippet for both, which is wrong for Page.
This is with User 1. The fact that the test did not pick it up must be a permission issue, like the test user does not have sufficient permissions? Unsure but it is wrong anyway.
Comment #89
bircherok, this time fixing the test to catch the other date as well.
Comment #90
jhodgdonGreat work!
So, this patch is very good. A few small things to fix:
I am not very happy with this. Can we instead just do something below like
and get rid of $item_count?
Here we should be able to use $type instead of loading the bundle class again?
Coding standards: move the { up to the previous line, and there should be a space after if before the (
drupal_render() is deprecated... I realize this is just moving a couple of lines to a new location, but it would be good to get rid of it, using dependency injection in the constructor.
Take a look at https://www.drupal.org/node/1354#functions to see how this should be formatted and documented. Some notes:
- First line should start with "Removes" not "remove"
- @param and @return need documentation
- @return should not say $build
- Add a paragraph explaning why we are doing this. Maybe:
This information is being removed from the rendered node that is used to build the search result snippet. It just doesn't make sense to have it displayed in the snippet.
Whoops.
This comment runs over 80 characters, and the next one too. Wrap to 80 characters (code can be longer but not comments).
Comment #92
bircherAddressing the review comments.
Comment #94
bircherfix typo in variable name... thank you test bot.
Comment #95
jhodgdonOne very minor note:
I think the reason this didn't help PHPStorm figure out this variable is that it needs to say
(from looking at other uses in Core).
Want to try that? Anyway that is how @var single-line comments are elsewhere in Core. Other than that, I think this is RTBC. Thanks!
Comment #96
bircherYes this fixes the notice.
Comment #97
jhodgdonNice! Thanks for finding/fixing that indentation problem I missed too. ;)
I think that 8 years and 9 months is long enough for this bug to stick around. Let's get this in!
Added beta eval, since this is a normal bug.
Comment #98
alexpottNeed to update @params documentation.
Let's make the pre_render called static so that we don't have to serialize
$this
. This is consistent with other places we do this - especially since the callback does not need any of the dependencies.This comment is overlong and slightly repetitive.
Comment #99
bircher1. thanks for noticing this oversight
2. good idea.
3. removed the repetition.
Comment #102
bircherComment #103
jhodgdonDoes this actually work?
Normally I think when we pass class/method names to PHP I think we drop the initial \ from namespaces?
Hm, but we have tests now and they pass, so I guess it is OK. Thanks for the fixes; all looks good to me. Again, RTBC.
Comment #104
bircherYes it works both ways. I was not sure how to write it, so I searched the code base for
['#pre_render'][] = '
and the current two places that this turns up a static method call is in TextTrimmedFormatter.php on line 79 and ViewsBlockBase.php on line 127. Both include the \ so for the sake of consistency I made the patch in the same way.Comment #105
xjmI love that we are fixing an issue that is almost 9 years old! I definitely hacked around this on more than one site.
Thanks for the beta evaluation and the test-only patch. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes.
I like that this also makes the code more legible and IDE-friendly as a side effect. :)
The SafeMarkup call is a no-no, but it's not introduced by this patch, so out of scope to fix it. It should be fixed in its own time in #2280965: [meta] Remove every SafeMarkup::set() call.
Entertaining aside: The credit form for this issue has as options what is basically a roll call of core committers past and present.
Committed and pushed to 8.0.x -- finally!
And then, anticlimactically... moving to D7 for backport. IMO, this is one of those issues that really does not make sense to do the backport in the same issue. See #2462101: [policy, no patch] Update backport policy to account for 8.x minor versions and 7.x divergence. But it's our current policy so following it for now.
Comment #107
hass CreditAttribution: hass commentedComment #111
stefan.r CreditAttribution: stefan.r commentedComment #112
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedPatch for D7. Updates to patch #73.