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

Reference: https://www.drupal.org/core/beta-changes
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.
CommentFileSizeAuthor
#112 70722-112.patch2.85 KBaerozeppelin
#99 interdiff-70722-96-99.txt2.56 KBbircher
#99 70722-99.patch8.58 KBbircher
#96 interdiff-70722-94-96.txt1.09 KBbircher
#96 70722-96.patch8.33 KBbircher
#94 interdiff-70722-92-94.txt601 bytesbircher
#94 70722-94.patch8.33 KBbircher
#92 interdiff-70722-89-92.txt6.93 KBbircher
#92 70722-92.patch8.33 KBbircher
#89 interdiff-70722-84-89.txt3.6 KBbircher
#89 70722-89.patch5.1 KBbircher
#89 70722-89-test-only.patch2.55 KBbircher
#88 adminshows.png16.25 KBjhodgdon
#84 remove-date-submitted-70722.patch2.09 KBpetar.gnjidic
#83 70722-83.patch3.54 KBbircher
#83 70722-83-test-only.patch2.57 KBbircher
#73 search-post-info-test-only-70722-73.patch1.43 KBdrupal_was_my_past
#73 search-post-info-with-test-70722-73.patch2.28 KBdrupal_was_my_past
#72 search-post-info-test-only-70722-72.patch1.41 KBdrupal_was_my_past
#72 search-post-info-with-test-70722-72.patch2.24 KBdrupal_was_my_past
#66 70722-reroll.patch5.99 KBjhodgdon
#53 search_results.patch6.13 KBrobertDouglass
#50 search_result_node_info_10.patch5.87 KBpfournier
#41 search_result_node_info_9.patch3.26 KBDavid Lesieur
#37 search_result_node_info_8.patch2.19 KBPasqualle
#34 search_result_node_info_7.patch1.66 KBPasqualle
#33 search_result_node_info_6.patch1.62 KBDavid Lesieur
#32 70722.patch965 bytesdouggreen
#28 search_result_node_info_5.patch1.67 KBDavid Lesieur
#25 search_result_node_info_4.patch1.53 KBDavid Lesieur
#22 search_result_node_info_3.patch3.48 KBDavid Lesieur
#21 search_result_node_info_2.patch3.48 KBDavid Lesieur
#18 search_result_node_info_1.patch3.48 KBDavid Lesieur
#17 search_result_node_info_0.patch3.48 KBDavid Lesieur
#14 search_result_node_info.patch3.13 KBDavid Lesieur
#9 search.module_display_result_settings_1.patch3.08 KBparanojik
#7 search.module_display_result_settings_0.patch3.22 KBparanojik
#4 search.module_display_result_settings.patch5.64 KBparanojik
search.module_display_results_settings.patch.txt5.34 KBhutch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sugree’s picture

search keywords module is not search.module.

Tobias Maier’s picture

Project: Search Keywords » Drupal core
Version: 4.7.x-1.x-dev » x.y.z
Component: Code » search.module

we only accept new features against drupal CVS HEAD (the upcoming 4.8)

Tobias Maier’s picture

Status: Needs review » Needs work

a lot of changes were made between drupal HEAD and 4.7
--> i guess it wont apply

paranojik’s picture

Status: Needs work » Needs review
FileSize
5.64 KB

reapplied

Dries’s picture

Status: Needs review » Needs work

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.

Steven’s picture

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?

paranojik’s picture

Status: Needs work » Needs review
FileSize
3.22 KB

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.

Dries’s picture

Status: Needs review » Needs work

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.

paranojik’s picture

It's back on the search settings page...

paranojik’s picture

Status: Needs work » Needs review

...and ready for review...

David Lesieur’s picture

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.

David Lesieur’s picture

Version: x.y.z » 6.x-dev

A bit late to try to revive this issue, but who knows? ;-)

David Lesieur’s picture

Title: Additional settings to search.module » Post information display setting for search results
David Lesieur’s picture

Re-rolled the patch (not taking into account my comment in #11).

Gurpartap Singh’s picture

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?

David Lesieur’s picture

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. ;-)

David Lesieur’s picture

This new patch takes theme settings into account and uses a proper variable name.

David Lesieur’s picture

Removed an unwanted whitespace.

David Lesieur’s picture

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! :-)

Steven’s picture

Status: Needs review » Needs work
    '#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.

David Lesieur’s picture

Assigned: hutch » David Lesieur
Status: Needs work » Needs review
FileSize
3.48 KB

Fixed!

David Lesieur’s picture

Umm, use this patch instead.

David Lesieur’s picture

Version: 6.x-dev » 7.x-dev

Targetting D7, although the patch still works on D6 at the moment.

Gurpartap Singh’s picture

Version: 7.x-dev » 6.x-dev
Category: feature » bug

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.

David Lesieur’s picture

Good point! Here's the patch. ;-)

Gurpartap Singh’s picture

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:

  if ($type != 'node' || theme_get_setting('toggle_node_info_'. $item['node']->type)) {
    // do the post info. stuff
  }

Hopefully, this is getting in soon. :)

Gábor Hojtsy’s picture

Brilliant idea to apply the node toggles here, it is in fact a bugfix this way :) A line of comment would be good though.

David Lesieur’s picture

Added a comment.

Gurpartap Singh’s picture

If the patch still applies, anyone going to commit? :)

David Lesieur’s picture

It still applies. ;-)

catch’s picture

Status: Needs review » Needs work

Not any more it doesn't.

douggreen’s picture

FileSize
965 bytes

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?

David Lesieur’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

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.

Pasqualle’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.66 KB

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

Pasqualle’s picture

Status: Reviewed & tested by the community » Needs work

no, sorry it does show php notice for test #2

Pasqualle’s picture

search-result.tpl.php

* Since $info_split is keyed, a direct print of the item is possible.
* This array does not apply to user searches so it is recommended to check
* for their existance before printing. The default keys of 'type', 'user' and
* 'date' always exist for node searches. Modules may provide other data.

Pasqualle’s picture

this is the minimal set of keys required.

Pasqualle’s picture

Status: Needs work » Needs review
David Lesieur’s picture

@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:

This array does not apply to user searches so it is recommended to check for their existance before printing

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.

Pasqualle’s picture

read further please

The default keys of 'type', 'user' and 'date' always exist for node searches.

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.

David Lesieur’s picture

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.

robertDouglass’s picture

Version: 6.x-dev » 7.x-dev
robertDouglass’s picture

still applies.

David Lesieur’s picture

LIVE FROM THE MINNESOTA SEARCH SPRINT: This patch still applies and works.

puregin’s picture

Title: Post information display setting for search results » Search module may leak post information

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.

puregin’s picture

Status: Needs review » Reviewed & tested by the community

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.

Dries’s picture

puregin: can you try writing a simpletest for this? It shouldn't be too hard.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

puregin: can you try writing a simpletest for this? It shouldn't be too hard.

douggreen’s picture

Title: Search module may leak post information » Search results should not expose private information
pfournier’s picture

Status: Needs review » Needs work
FileSize
5.87 KB

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.

pfournier’s picture

Status: Needs work » Needs review
lilou’s picture

Patch need to be rolled.

robertDouglass’s picture

Status: Needs work » Needs review
FileSize
6.13 KB

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:

      // 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.

Anonymous’s picture

Code looks good; need to test.

Status: Needs review » Needs work

The last submitted patch failed testing.

mdlueck’s picture

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

Pasqualle’s picture

@mdlueck: I think in D5 it is possible to override the theme function of the search result form.

mdlueck’s picture

@Pasqualle: Thanks for the reminder. Implemented!

Status: Needs work » Needs review

Re-test of search_results.patch from comment #53 was requested by sun.

Status: Needs review » Needs work

The last submitted patch, search_results.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

#53: search_results.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, search_results.patch, failed testing.

jhodgdon’s picture

Title: Search results should not expose private information » Search results should only show post info if content type does

Looks like we need a reroll of this patch.

goofrider’s picture

Any progress with the patch?

jhodgdon’s picture

Assigned: David Lesieur » Unassigned

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".

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
5.99 KB

Here's a reroll of this patch.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Assume the test bot agrees, I think we should get this into D7, and then backport to D6.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 70722-reroll.patch, failed testing.

jhodgdon’s picture

These failures are in Search module test cases, so they look real. Will need to investigate...

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

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".

bakr’s picture

Status: Needs work » Needs review

#18: search_result_node_info_1.patch queued for re-testing.

drupal_was_my_past’s picture

Trying 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.

drupal_was_my_past’s picture

drupal_was_my_past’s picture

Assigned: Unassigned » drupal_was_my_past
kscheirer’s picture

Status: Needs work » Needs review

#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.

Status: Needs review » Needs work

The last submitted patch, search-post-info-with-test-70722-73.patch, failed testing.

ianthomas_uk’s picture

Title: Search results should only show post info if content type does » Search results should respect the content type's "Display author and date information." option
Assigned: drupal_was_my_past » Unassigned
Issue summary: View changes

Rewrote 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.

ianthomas_uk’s picture

Status: Needs review » Needs work
hass’s picture

No, usernames must be removed. Otherwise the setting is useless!

jhodgdon’s picture

OK 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:

  $node_type_config = \Drupal::config('node.type.' . $node->bundle());
  $display_submitted = $node_type_config->get('settings.node.submitted');
  $node_type_config->set('settings.node.submitted', TRUE);

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...

jhodgdon’s picture

Issue tags: +Novice

Should be a good Novice issue?

petar.gnjidic’s picture

Assigned: Unassigned » petar.gnjidic

I'll take this on DDD2015

bircher’s picture

Status: Needs work » Needs review
FileSize
2.57 KB
3.54 KB

Sorry @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.

petar.gnjidic’s picture

Here is the partial patch.

jhodgdon’s picture

Hey 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.

The last submitted patch, 83: 70722-83-test-only.patch, failed testing.

jhodgdon’s picture

Looking 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:

title here this is a test admin [date it was created]

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?

jhodgdon’s picture

Status: Needs review » Needs work
FileSize
16.25 KB

Just 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.
Screen shot of search results

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.

bircher’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
5.1 KB
3.6 KB

ok, this time fixing the test to catch the other date as well.

jhodgdon’s picture

Status: Needs review » Needs work

Great work!

So, this patch is very good. A few small things to fix:

  1. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -305,12 +305,18 @@ protected function prepareResults(StatementInterface $found) {
    +    $item_count = 0;
    +
         foreach ($found as $item) {
    

    I am not very happy with this. Can we instead just do something below like

    $result = array(...);
    if () {
      $result['user'] = ...
    }
    $results[] = $result;
    

    and get rid of $item_count?

  2. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -325,23 +331,44 @@ protected function prepareResults(StatementInterface $found) {
             'type' => SafeMarkup::checkPlain($this->entityManager->getStorage('node_type')->load($node->bundle())->label()),
    

    Here we should be able to use $type instead of loading the bundle class again?

  3. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -325,23 +331,44 @@ protected function prepareResults(StatementInterface $found) {
    +      if($type->displaySubmitted())
    +      {
    +        $results[$item_count] += array(
    

    Coding standards: move the { up to the previous line, and there should be a space after if before the (

  4. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -325,23 +331,44 @@ protected function prepareResults(StatementInterface $found) {
    +            'user' => drupal_render($username),
    

    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.

  5. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -325,23 +331,44 @@ protected function prepareResults(StatementInterface $found) {
    +   * Remove the submitted by information from the build array.
    +   * @param array $build
    +   * @return array $build
    +   */
    +  public function removeSubmittedInfo(array $build) {
    

    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.

  6. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -656,4 +683,4 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
    \ No newline at end of file
    

    Whoops.

  7. +++ b/core/modules/search/src/Tests/SearchExactTest.php
    @@ -55,5 +59,24 @@ function testExactQuery() {
    +    // Check that with post settings turned on the post information is displayed.
    

    This comment runs over 80 characters, and the next one too. Wrap to 80 characters (code can be longer but not comments).

The last submitted patch, 89: 70722-89-test-only.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
6.93 KB

Addressing the review comments.

Status: Needs review » Needs work

The last submitted patch, 92: 70722-92.patch, failed testing.

bircher’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
601 bytes

fix typo in variable name... thank you test bot.

jhodgdon’s picture

Status: Needs review » Needs work

One very minor note:

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -145,12 +154,13 @@ static public function create(ContainerInterface $container, array $configuratio
     $this->moduleHandler = $module_handler;

I think the reason this didn't help PHPStorm figure out this variable is that it needs to say

/** @var \Drupal\node\NodeTypeInterface $type */

(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!

bircher’s picture

Status: Needs work » Needs review
FileSize
8.33 KB
1.09 KB

Yes this fixes the notice.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Nice! 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -145,12 +154,13 @@ static public function create(ContainerInterface $container, array $configuratio
    -  public function __construct(array $configuration, $plugin_id, $plugin_definition, Connection $database, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, Config $search_settings, LanguageManagerInterface $language_manager, AccountInterface $account = NULL) {
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, Connection $database, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, Config $search_settings, LanguageManagerInterface $language_manager, RendererInterface $renderer, AccountInterface $account = NULL) {
    

    Need to update @params documentation.

  2. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -310,11 +320,16 @@ protected function prepareResults(StatementInterface $found) {
    +      $build['#pre_render'][] = array($this, 'removeSubmittedInfo');
    
    @@ -325,23 +340,51 @@ protected function prepareResults(StatementInterface $found) {
    +  public function removeSubmittedInfo(array $build) {
    

    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.

  3. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -325,23 +340,51 @@ protected function prepareResults(StatementInterface $found) {
    +   * 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.
    

    This comment is overlong and slightly repetitive.

bircher’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
2.56 KB

1. thanks for noticing this oversight
2. good idea.
3. removed the repetition.

Status: Needs review » Needs work

The last submitted patch, 99: 70722-99.patch, failed testing.

hass queued 99: 70722-99.patch for re-testing.

bircher’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Does this actually work?

+      $build['#pre_render'][] = '\Drupal\node\Plugin\Search\NodeSearch::removeSubmittedInfo';

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.

bircher’s picture

Yes 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.

xjm’s picture

Assigned: petar.gnjidic » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

I 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.

  1. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -310,11 +340,16 @@ protected function prepareResults(StatementInterface $found) {
    +      /** @var \Drupal\node\NodeTypeInterface $type*/
    +      $type = $this->entityManager->getStorage('node_type')->load($node->bundle());
    
    @@ -325,18 +360,27 @@ protected function prepareResults(StatementInterface $found) {
    -        'type' => SafeMarkup::checkPlain($this->entityManager->getStorage('node_type')->load($node->bundle())->label()),
    ...
    +      if ($type->displaySubmitted()) {
    +        $result += array(
    +          'user' => $this->renderer->render($username),
    +          'date' => $node->getChangedTime(),
    +        );
    

    I like that this also makes the code more legible and IDE-friendly as a side effect. :)

  2. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -310,11 +340,16 @@ protected function prepareResults(StatementInterface $found) {
    -      $node->rendered = SafeMarkup::set(
    -        drupal_render($build) . ' ' .
    +      $rendered = SafeMarkup::set(
    +        $this->renderer->render($build) . ' ' .
             SafeMarkup::escape($this->moduleHandler->invoke('comment', 'node_update_index', array($node, $item->langcode)))
           );
    

    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.

  • xjm committed 6090185 on 8.0.x
    Issue #70722 by David Lesieur, bircher, rocket_nova, paranojik,...
hass’s picture

Version: 8.0.x-dev » 7.x-dev

  • xjm committed 6090185 on 8.1.x
    Issue #70722 by David Lesieur, bircher, rocket_nova, paranojik,...

  • xjm committed 6090185 on 8.3.x
    Issue #70722 by David Lesieur, bircher, rocket_nova, paranojik,...

  • xjm committed 6090185 on 8.3.x
    Issue #70722 by David Lesieur, bircher, rocket_nova, paranojik,...
stefan.r’s picture

Issue tags: -Novice
aerozeppelin’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.85 KB

Patch for D7. Updates to patch #73.

  • xjm committed 6090185 on 8.4.x
    Issue #70722 by David Lesieur, bircher, rocket_nova, paranojik,...

  • xjm committed 6090185 on 8.4.x
    Issue #70722 by David Lesieur, bircher, rocket_nova, paranojik,...