Rename and deprecate hook_ranking

+100. I mentioned this back in the issue to document it but we only got documentation, not a hook rename. I think based on the argument that it isn't part of search module, like you said. So moving this entire chunk of code to search.module makes a lot of sense, too.

Tagging "DX".

(Merging "node system" and "node.module" components for 8.x; disregard.)

So. In 8.x, I do not think we can change this any more. It might have gotten noticed earlier if it was in the search.module component rather than buried in Node, because the Search maintainers did not know about it. Anyway, moving there and bumping to 9.x at this point.

Regarding the issue:
- Node search is now a plugin that lives in Node module, and this hook is definitely only for node searches. So the hook definitely belongs in node module, not search module.
- I am in favor of renaming it hook_node_search_ranking().

I think as long as we don't break BC (leave the old hook name intact) it would be fine to add an additional hook fired at the same spot with an improved name. Not 100% sure, but tentatively switching back to 8.2.x.

Fair enough. Updating the issue summary, and I think this could then probably be a Novice issue, at least for some novice coders... seems pretty straightforward.

Seems interesting! This will be my first issue like this. I'm gonna try it.

Here I have worked and followed the steps provided from the description and branched-off from 8.2.x too.

Special emphasis on

• Item c: I hope the way I implemented the array addition of the old and new arrays is acceptable.
• Item d: Based from https://api.drupal.org/api/drupal/core!modules!node!node.api.php/functio... as stated on the description.
• Item e: I have also created a basic draft change record based from https://www.drupal.org/contributor-tasks/draft-change-record

Hope everything are in order.

Thanks!

Wow, looks perfect, on the first try! All of the tests also passed, and we have extensive tests for all the node search ranking options. Great work, thanks! I did make some small edits to the change record.

Oh.... One thing I forgot in the task list... I guess we need to file an issue for 9.x to remove the hook, and add a @todo comment to the line of code in NodeSearch that invokes the old hook. The comment should say something like:

@todo Remove this in 9.0. Issue: [link to the new issue]

Would you like to file that issue and make the small revision on the patch? Sorry for not thinking of it earlier. ;)

Maybe the line of code would be easier to remove if the order was changed too, so the new hook is invoked first, and the old hook's results are added in afterwards?

Thank you very much @jhodgdon! Was quite scary! :)

I will work on the additional tasks.

Hi @jhodgdon! Here I have added your recommendations from comment #9.

Tasks done

• File an issue for 9.x to remove the hook here https://www.drupal.org/node/2690437. I hope it's to add this to this ticket's related issue.
• Add a @todo comment to the line of code in NodeSearch that invokes the old hook.
• Maybe the line of code would be easier to remove if the order was changed too, so the new hook is invoked first, and the old hook's results are added in afterwards?

Hope everything is in order.

Thanks!

Great! The new issue looks good. I made these two issues related (on the other one), and also edited the change record to reference the second issue (a change record can reference more than one issue).

By the way, a better way to make a link to an issue when you are editing a page or comment on drupal.org is to put something like #2690437: Remove the deprecated hook_ranking() and the code invoking it in the text. So instead of a simple link like https://www.drupal.org/node/2690437, you get a link that looks like #2690437: Remove the deprecated hook_ranking() and the code invoking it... a nice trick. ;)

And... Just one more small suggestion on the patch:

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -736,14 +736,17 @@ protected function parseAdvancedDefaults($f, $keys) {
+      // Add the results together using array addition.
+      $this->rankings = $this->moduleHandler->invokeAll('node_search_ranking');
+      // @todo Remove this in 9.0. Issue: https://www.drupal.org/node/2690437.
+      $this->rankings += $this->moduleHandler->invokeAll('ranking');

I don't think we need a comment saying we are adding results together using array addition. We generally want in-code comments that tell us why things are being done they way they are, not comments that just tell us what the code is doing if it's pretty obvious (as I think it is here).

So... I think just take out the first comment completely.

For the second one, maybe instead of "Remove this" say "Remove backwards-compatible invocation of deprecated hook", so it's clearer why we are removing the line?

Thanks! You're doing great!

Also added link to new issue to issue summary, for completeness.

Thanks for your review and encouragement @jhodgdon!

That link trick is awesome! A very good shortcut and more informative. I'll definitely use that. :) Got it on the "tell us why things are being done they way they are" rather than "comments that just tell us what the code is doing" too. I'll keep that in mind.

I will work on these recommended changes.

Here is the recommendations from @jhodgdon.

Things done

• Removed the previous comment for the new hook's line of code.
• Updated the @todo description.

Thanks!

Looking great!

So... I took one final look through this and found a couple of small things to fix, that I (sorry!) missed in my earlier reviews... and then I think we'll be done.

1. +++ b/core/modules/node/node.api.php
   @@ -442,6 +442,9 @@ function hook_node_update_index(\Drupal\node\NodeInterface $node) {
   + * @deprecated in Drupal 8.2.0 and will be removed before Drupal 9.0.0. Use
   + * hook_node_search_ranking() instead.
   

   The second line here needs to be indented 2 spaces.

2. +++ b/core/modules/node/node.api.php
   @@ -469,6 +472,73 @@ function hook_ranking() {
   +        // Note that we use i.sid, the search index's search item id, rather than
   +        // n.nid.
   

   As long as we are fixing things...

   item id => item ID

   Also the first comment line here exceeds 80 characters, so can you rewrap these two lines?

3. +++ b/core/modules/node/node.api.php
   @@ -469,6 +472,73 @@ function hook_ranking() {
   +        // The highest possible score should be 1, and the lowest possible score,
   +        // always 0, should be 0.
   

   And rewrap this too, please.

4. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
   @@ -751,14 +751,17 @@ protected function parseAdvancedDefaults($f, $keys) {
   +      // @todo Remove backwards-compatible invocation of deprecated hook in 9.0.
   +      //   Issue: https://www.drupal.org/node/2690437.
   

   I don't think we should indent "Issue:" in the second line here.

Thanks for your review and no problem at all! :) I'll work on these items.

Here I have applied the recommendations from @jhodgdon.

Thanks!

Thanks! I think this is ready to go now.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -751,14 +751,17 @@ protected function parseAdvancedDefaults($f, $keys) {
+      $this->rankings = $this->moduleHandler->invokeAll('node_search_ranking');
+      // @todo Remove backwards-compatible invocation of deprecated hook in 9.0.
+      // Issue: https://www.drupal.org/node/2690437.
+      $this->rankings += $this->moduleHandler->invokeAll('ranking');

Are we sure that this is the right way to merge the results? Inside invokeAll it does a $return = NestedArray::mergeDeep($return, $result);... I think this code should look more like this.

I think we should be doing

$this->rankings = $this->moduleHandler->invokeAll('ranking');
$this->rankings = NestedArray::mergeDeep($this->rankings, $this->moduleHandler->invokeAll('node_search_ranking'));

This way the new hook is preferred and results are merged correctly. We still have a theoretical problem - say moduleX was changing the ranking the node module provides by implementing hook_ranking and having a higher weight than node... this change will break that.... hmmm... so maybe

$this->rankings = $this->moduleHandler->invokeAll('node_search_ranking');
$this->rankings = NestedArray::mergeDeep($this->rankings, $this->moduleHandler->invokeAll('ranking'));

is correct.

Also as we're implementing a bc layer we should a test of it.

For testing: I suggest that we leave the comment module using the deprecated old hook for now, since we have coverage for comment, node, and statistics module hooks already.

For merging: for this particular hook I cannot see a reason that we need to do anything more than a simple array addition.

@jhodgdon I think that rather than leaving a deprecated usage around we should add an explicit test - we should be able to add the implementation to the node_test module.

Also we can't do simple array addition because that's not what is done internally in ModuleHandler::invokeAll()

I'm removing the novice tag on this issue temporarily, and I added the needs issue summary update because the proposed resolution should be updated based on alexpott's comments regarding adding a test.

Moving back to node module as per #3 and to reflect the patch. Ultimately I think search related functionality should be removed from node module, particularly if search gets deprecated in core, in favour of search api in drupal cms.

I agree with moving search stuff to the search module, but it looks like this hook is directly invoked from the node_search search plugin, so maybe it does belong in node? Who knows how often this stuff is actually used though, maybe we can eventually move node search stuff to a node_search contrib module.

In any case, this needs a reroll on to an MR which is going to require quite a bit of rework for OOP hooks I imagine.

$this->rankings += $this->moduleHandler->invokeAllDeprecated('Use hook_node_search_ranking() instead. See https://www.drupal.org/node/2690393.', 'ranking');

Nice! not see that one before.

CR is straight forward and seems like a good deprecation and conversion. Don't see anything off.

Committed 85aac9b and pushed to 11.x. Thanks!

Since this deprecates a hook, I'd love opinions on an issue to change how we do that: #3001190: Deprecate ModuleHandlerInterface::*Deprecated() in favor of #[DeprecatedHook]