Move node_mass_update_* functions to service and fix invalid hook invocation in NodeHooks

Fun fact! I first found the oddity that is node mass update here 📌 [ignore] Convert everything everywhere all at once Active which is why I knew it wasn't a hook in the first place even though it goes through through module handler invoke.

There are not many of these types of calls left in core.

This is barely used in contrib, it would be nice if we could overhaul this with something more modern but maybe that could be done in follow-ups http://codcontrib.hank.vps-private.net/search?text=node_mass_update%28%2...

Speaking internally, @larowlan suggested changing the $updates array to a static closure that takes the loaded $node and would allow us to just call API directly. I think this would be a huge improvement and would be good for a follow up.

We chatted about naming internally and landed on NodeBulkUpdate::process, the word "mass" doesn't really get used a lot in Drupal so this seemed to fit better.

Setting to NR to get some eyes on it, agree on the naming, then I'll fix up the CR and make any further changes.

All tests for this are in core/modules/user/tests/src/Functional/UserCancelTest.php

Any concern with removing in 12? Should it be pushed to 13?
Seems odd the test coverage is in user should it be moved to the node module instead?
Think process() makes sense.

Hope that helps some.

Bumped to D13.

The tests could really live in either place I guess because technically it's testing user module hooks in Node module, but the test also tests the same stuff around comment so it makes sense to have them all live together in user?

This looks great!

I like the name process.
Revision changes look good too.

The tests should stay in user I think since that is the caller. We could always move it in a follow up.

I did wonder if there is any reason not to move the deprecated function to node.module so we could delete the admin.inc file?

It's not really super important so feel free to push back.

I did wonder if there is any reason not to move the deprecated function to node.module so we could delete the admin.inc file?

If we follow what we're now doing with the template_preprocess callbacks it would be to keep the file and add a @trigger_error(), as this is named admin.inc it's also blocked on the loadAllIncludes() issue then.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Made a small tweak to the CR but believe this one is good to go. Don't see any open items.

Committed/pushed to 11.x, thanks!

catch → closed merge request !12561