Problem/Motivation

node.admin.inc is 180 lines of procedural code that should be moved to a service/class.

@berdir/@nicxvan also found in https://www.drupal.org/project/drupal/issues/3532204#comment-16170601 that node_mass_update is being invoked as if it were a hook in NodeHooks::userCancelBlockUnpublish/userCancelReassign which needs to change as these are not hooks.

Steps to reproduce

N/A

Proposed resolution

Move all functions in node.admin.inc to a class/service
Fix documentation
Deprecate node_mass_update

Make some minor refactors to the functionality, this is covered in https://git.drupalcode.org/project/drupal/-/merge_requests/12561/diffs?c...

Remaining tasks

Agree on naming
Review https://git.drupalcode.org/project/drupal/-/merge_requests/12561

User interface changes

N/A

Introduced terminology

N/A

API changes

node_mass_update is deprecated
New NodeBulkUpdate service to mimic node_mass_update functionality.

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#11 3533083-nr-bot_b5a7gw_6.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3533083

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

acbramley created an issue. See original summary.

nicxvan’s picture

nicxvan commented

Fun fact! I first found the oddity that is node mass update here #3483599: Convert all procedural hook implementations to Hook classes 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.

Just missed the include bit that @berdir always seems to catch!

acbramley’s picture

acbramley commented
Issue summary: View changes

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.

acbramley opened merge request !12561

acbramley’s picture

acbramley commented
Issue summary: View changes
Status: Active » Needs review

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

acbramley’s picture

acbramley commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

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?

nicxvan’s picture

nicxvan commented

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3533083-nr-bot_b5a7gw_6.txt91 bytes

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.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

  • catch committed 8395afd9 on 11.x 
    Issue #3533083 by acbramley, nicxvan, smustgrave, berdir: Move...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !12561

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.