Move preprocess functions into NodeThemeHooks

I compared the changes from node.module to NodeThemeHooks with the following commands:

diff -upb \
  <(git show 11.x:core/modules/node/node.module | sed -n '208,256p') \
  <(git show 3532204-move-preprocess-functions:core/modules/node/src/Hook/NodeThemeHooks.php | sed -n '208,256p') \
  >> node-theme-hooks.patch

diff -upb \
  <(git show 11.x:core/modules/node/node.module | sed -n '260,273p') \
  <(git show 3532204-move-preprocess-functions:core/modules/node/src/Hook/NodeThemeHooks.php | sed -n '64,78p') \
  >> node-theme-hooks.patch
  
diff -upb \
  <(git show 11.x:core/modules/node/node.module | sed -n '277,374p') \
  <(git show 3532204-move-preprocess-functions:core/modules/node/src/Hook/NodeThemeHooks.php | sed -n '106,203p') \
  >> node-theme-hooks.patch

Then looking at node-theme-hooks.patch, which I've attached, I can see the only changes are:

• function names
• addition of hook attributes
• indentation
• use of dependency injection

I also grepped for each of the removed functions in case there were string references in comments and found 26 in 11.x and 4 in this branch.

$ git grep -E 'template_preprocess_node_add_list|node_preprocess_html|node_preprocess_block|node_theme_suggestions_node|template_preprocess_node' 11.x | wc -l
26
$ git grep -E 'template_preprocess_node_add_list|node_preprocess_html|node_preprocess_block|node_theme_suggestions_node|template_preprocess_node' 3532204-move-preprocess-functions | wc -l
4

So the following will need to be updated:

$ git grep -E 'template_preprocess_node_add_list|node_preprocess_html|node_preprocess_block|node_theme_suggestions_node|template_preprocess_node' 3532204-move-preprocess-functions
3532204-move-preprocess-functions:core/lib/Drupal/Core/Render/theme.api.php:  // This example is from node_preprocess_html(). It adds the node type to
3532204-move-preprocess-functions:core/lib/Drupal/Core/Render/theme.api.php: * function node_theme_suggestions_node(array $variables): array {
3532204-move-preprocess-functions:core/lib/Drupal/Core/Render/theme.api.php: * '#theme' => 'node__article' is called, then node_theme_suggestions_node()
3532204-move-preprocess-functions:core/lib/Drupal/Core/Render/theme.api.php: * will be invoked, not node_theme_suggestions_node__article(). The specific

Thanks @mstrelan!

The node_preprocess_html comment is legit, I'll fix that.

The other ones are actually just example documentation of these hooks. That whole thing needs an update because it says things like "Implementations of this hook must be placed in *.module or *.theme files", and IMO should not use core modules as examples but that's of course for another issue.

The line about node_theme_suggestions_node is just describing how these suggestions are called, not specifically referencing node's implementation of the hook.

Great. I also checked that all of the services injected in to the new class have been used. I found another issue that had injected services but not used them, our phpstan config doesn't detect that. This is RTBC as per #5 and #6.

Created 📌 theme.api.php needs updates now that hooks can be OO Active for the docs issues.

These are the last hooks you can add the container parameter.

parameters:
node.skip_procedural_hook_scan: true

Hmm adding back the old functions has broken tests in weird ways Call to a member function getOwnerId() on null

Turns out it was the procedural hook skip that caused the failures.

@nicxvan any idea what hook is remaining that's causing that? I can't see any

This is ready again.

I confirmed the template preprocess are deprecated now. There are literally no uses in contrib for it and we don't have to technically deprecate them, but since people do occasionally call them we've been deprecating template preprocess.

I spent over an hour trying to figure out why marking node as converted breaks core/modules/jsonapi/tests/src/Functional/UserTest.php and another jsonapi test.

I followed these principles:

1 identify the failing test and review for anything obvious
2 checkout 11.x and one by one break the hooks you converted by changing the function name and run the test see if it fails on any (note this can be a false negative)  this can identify any that were converted incorrectly or have some other weird dependency like system_theme did
3 in the branch you coverted on move all procedural functions to node.module and add the proceduralhookscanstop attribute starting from the bottom (this will identify the function that is causing the test to fail)
4 slowly go insane until you realize a hook was documented incorrectly so it converted to the wrong module (views)
5 Add a file_put_contents that outputs gathered hooks, run the test with the skip and without and see what is different

None of these provided a hint.

I even grabbed every function in the three files removed node and grepped for something calling modulehandler invoke with the function name and nothing came back beyond node_mass_update which isn't a real hook.

hook_node_grants is INVOKED by some of the functions, but that shouldn't matter.

I think we move marking this to a follow up.

Oh, a challenge, can't resist a challenge.

It's user cancel, so I assumed it's related to the user_cancel hook in node module and from there it was pretty straight forward to identify.

\Drupal\node\Hook\NodeHooks::userCancelBlockUnpublish uses ModuleHandler::invoke() to call node_mass_update. that's not a hook, but it misuses that to load the function in the include file.

This works:

$this->moduleHandler->loadInclude('node', 'admin.inc');
      node_mass_update($vids, ['uid' => 0, 'revision_uid' => 0], NULL, TRUE, TRUE);

Agreed that this is out of scope, but lets open an issue for it. There is a risk that we break similar calls though.

Crediting you both for the sleuthing, I'll open the issue

Crediting @mstrelan for the excellent review in #5

📌 Move node_mass_update_* functions to service and fix invalid hook invocation in NodeHooks Active for #13

Oh of course that is why the attribute worked cause I moved it to the module file to make changing position easier.

Btw there is a duplicate about investigating mass_update.

🐛 Fatal error when passing NULL to Renderer::render() Active isn't RTBC yet, but I'd suggest we hold this until that's in as that will need to be backported and having template_preprocess_node() converted to OOP will complicate that.

Makes sense