Remove NodeHooks1

It's not a 100% duplicate, e.g. query_node_access_alter only appears to be in this file. Might be necessary to merge it with NodeHooks.

Hi @catch
Is the file name "nodehooks1"?
I couldn't find it. Could you tell me where it is located?

I don't think it's a duplicate at all? The original NodeHooks.php was added in 📌 OOP hooks using event dispatcher Needs review , I believe as a demonstration of implementing the same hook twice (hook_user_cancel). The newer NodeHooks1.php was added in 📌 [ignore] Convert everything everywhere all at once Active when all the remaining hooks were converted. I think we should move the 2 hooks in NodeHooks to NodeHooks1, delete NodeHooks and then rename NodeHooks1 to NodeHooks. Any further refactoring should be done later.

Sorry, didn't see the Novice tag. Should have left it.

Given it's not a duplicate it's also not a novice issue.

So re-ran a few times but some performance tests are consistently failing

Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest::testLogin

I put a breakpoint in \Drupal\Tests\PerformanceTestTrait::collectPerformanceData and compared $performance_test_data['cache_operations'] between HEAD and this issue. These are the 2 additional cache gets:

{
  "operation": "get",
  "cids": "entity_type_definitions.installed",
  "bin": "discovery",
  "start": 1741650201.981434,
  "stop": 1741650201.984118
},
{
  "operation": "get",
  "cids": "node.field_storage_definitions.installed",
  "bin": "discovery",
  "start": 1741650201.984128,
  "stop": 1741650201.985614
}

Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testStandardPerformance

The diff in the array comparison is:

[
    2 => 'SELECT "cid", "data", "created", "expire", "serialized", "tags", "checksum" FROM "cache_discovery" WHERE "cid" IN ( "entity_type_definitions.installed" ) ORDER BY "cid"',
    3 => 'SELECT "cid", "data", "created", "expire", "serialized", "tags", "checksum" FROM "cache_discovery" WHERE "cid" IN ( "node.field_storage_definitions.installed" ) ORDER BY "cid"',
]

So it looks like both tests are failing for the same reasons, i.e. entity_type_definitions.installed and node.field_storage_definitions.installed.

I would imagine this is because of dependency injection. Previously we were injecting the entity type manager and loading node storage in NodeHooks.php, but this was only ever invoked for hook_user_cancel. Now that this is combined with NodeHooks1 we're injecting these for far more hooks. I think in this case we can simply update the performance test expectations, but this also highlights why it might be worth splitting hooks classes based on what dependencies are required. Alternatively, we could revert to using Drupal::service here to reduce the number of cache gets here.

Also was running in to some other issues but they seem to be fixed in 📌 Remove cache tag checksum assertions from performance tests Active , so I merged in 11.x again.

Update to performance tests appear to fix everything.

acbramley → changed the visibility of the branch 11.x to hidden.

Based on the description in #10 I don't know if I agree with combining these all into 1 class.

We now have 21 hooks in a single class, 19 of which don't need the injected services (all from NodeHooks1).

IMO we could just rename NodeHooks1 to something a bit more descriptive? We don't have to shove every single hook into a single class.

Need an IS update too as it's not a duplicate file.

One idea I had was whether we could make all hooks that don't use services into static functions, and then we could group those hooks into NodeStaticHooks. I've tested this and it does seem to work, however I don't know if there's any performance benefit from making them static, but it seems like it might?

Closed 📌 Rename NodeHooks1 to something meaningful Active as a duplicate.

Yes #10 makes this tricky, we should at least make sure the extra service instantiations aren't in the critical path.

Just a quick note, this only exists because of a file exists when running rector the script creates a new file and increments. When I ran core we had only created the one core conversion in NodeHooks so NodeHooks1 was created.

There is no duplication it is just manual vs automated conversions.

Thanks @nicxvan, that's the same conclusion I came to in #4. I've updated the IS to clarify.

I've pushed another branch to see how the tests look if we remove dependency injection in this class. There are already plenty of other services in the class accessed by \Drupal::*.

No need to do this in two steps, I think we're leaning towards naming the file based on the api where the hook is defined.

Then we can add DI for each.

If there is a problematic one we can split further. I've added the meta.

OK that's good to know. I had another idea that we could use service closures and the #[AutowireServiceClosure] attribute in hook classes. I replaced every \Drupal:: call in NodeHooks with a service closure and re-ran the performance tests. This resulted in the same numbers as when using the static everywhere. Then I switched to directly injecting all the services and ran the tests again, expecting a worse score, but again it was the same as using static everywhere. Not sure what that tells us, injecting some services results in a performance regression but injecting all services does not. I couldn't get all the tests passing though, so maybe I've broken something.

Sorry, we do need to split the file though, we don't want to be injecting everything on every hook.

Totally agree. The point of that MR was to compare performance. For some reason MR !11406 requires adjustments to existing performance tests when injecting two services, but that same test passes in MR !11511 without adjustments when injecting all services. So I'm trying to figure out why there is a discrepancy.

mstrelan → changed the visibility of the branch 3511357-nodehooks-di to hidden.

I figured out why there was a performance regression in some branches but not others. In the first MR I had kept the node storage load in the constructor, even after I commented that it was an anti-pattern. But in the additional branches I had abstracted that out and only loaded when needed.

I've updated the original MR to not load the node storage in the constructor and reverted the expected performance test metrics to the original values.

Going to leave it there to decide if we want to proceed with this or split things further as per #21.

Glad you sorted that out!

I think my general recommendation is going to be a 3 part solution for all hooks.

1. Rough conversion, i.e. convert as close to exact as possible, (lift and shift) This is done for almost all hooks in core.
2. Rough organization, DI, t(), Split out by *.api.php, add DI, convert to $this->t()
3. Fine tuning - individual refactors and split out critical path performance hooks

I am going to catalog the breakdown for the other issue and move these over. Since this is step 2 I'd advocate we split the files out by api.php

If we split by api.php it will look like this:

NodeConfigTranslationHooks
Hooks: configTranslationInfoAlter
Services: n/a

NodeCoreHooks
Hooks: cron
Services: module_handler, entity_type_manager, state

NodeDatabaseHooks
Hooks: queryNodeAccessAlter
Services: current_user, module_handler, entity_type_manager, node.grant_storage, renderer

NodeEntityHooks
Hooks: userPredelete, configurableLanguageDelete, commentInsert, commentUpdate, commentDelete, entityViewDisplayAlter, entityExtraFieldInfo, nodeAccess
Services: entity_type_manager
Note: Only the first two hooks need the service

NodeHelpHooks
Hooks: hook_help
Services: current_user, messenger

NodeMenuHooks
Hooks: localTasksAlter
Services: n/a

NodeModuleHooks
Hooks: modulesInstalled, modulesUninstalled
Services: moduleHandler

NodeNodeHooks
Hooks: ranking
Services: state

NodeThemeHooks
Hooks: pageTop, formSystemThemesAdminFormAlter, theme
Services: current_route_match, form_builder, config.factory
Note: pageTop doesn't require config.factory but it would be injected pretty much everywhere. Also theme has no requirements.

NodeUserHooks
Hooks: userCancelBlockUnpublish, userCancelReassign
Services: entity_type_manager, module_handler

So we will be splitting to 10 new classes. Some of these make a lot of sense, like NodeModuleHooks. But NodeEntityHooks looks like it could benefit from splitting further, e.g. userPredelete could go to NodeUserHooks so we dont need entity_type_manager there.

I don't really have an opinion on the matter, but if it's agreed this is what we should do then I can work on it.

Wow that was quick!
I think that looks great!

But NodeEntityHooks looks like it could benefit from splitting further, e.g. userPredelete could go to NodeUserHooks so we dont need entity_type_manager there.

That would be part of step 3.
I don't have a super strong opinion, but I think keeping it to three steps will reduce bike shedding while getting the improvements we want in a reasonable time.

For example, I wouldn't put those with user hooks since they are not part of that, I'd make it part of another class like: NodeEntityHooks and NodeEntityDeleteHooks, then I would add the three delete hooks even if only two need the injection.

Just throwing my 2cents in but I really like how 📌 Clean up hook implementations in the Taxonomy module Active got organized. Maybe can do something similar.

29 is following the same logic as 📌 Clean up hook implementations in the Taxonomy module Active

Let’s do that then :)

Nevermind. I talked with larowlan and found out I thought this was not already in a minor release. sorry for the noise.

acbramley → changed the visibility of the branch 3511357-remove-nodehooks1 to hidden.

acbramley → changed the visibility of the branch 3511357-remove-nodehooks1-2 to hidden.

Split by api.php as per #29

FYI I haven't changed anything to do with DI except in NodeModuleHooks where both hooks need the module handler.

I'm not sure how we should handle it, we've seen impacts to performance tests in previous changes and I've also seen how they can break Kernel tests where a test doesn't install the module that implements the service being injected, since the test doesn't hit the hook it's not necessary.

Maybe we should defer the DI discussion to another issue and get the organisation of hooks in first since it's going to be quite disruptive already.

@acbramley I don't think DI have been blocker for these tickets. @nicxvan may know the ticket where that's being tracked?

Sounds like good novice tasks for a dev day to go back and add DI to these conversions!

1 comments I do want to make, not 100% phpstan and all is running on hook folders yet but will it ding the empty docs for __construct()? Not a blocker just mentioning.

Looking at the MR seems like a good conversion though

Constructors do not need documentation if everything is typed I think.

I agree DI should not be a blocker.

Some bikeshedding in my review comments.

So should this one and the taxonomy one be postponed till an agreed upon structure can be decided?

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.

Rebased this and fixed up a bunch of conflicts, copying across some changes to hook_help that made it in in the meantime.

Feel this is one could be endless chasing perfection. Believe what's currently there is definitely better then present. If a standard (apologize if one has been made and I don't know) think this is good.

Going to go on a limb and mark.

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.

Restoring from #50

Added a few thoughts, not changing the status for now if you want to wait for core committer feedback/decisions.

I've consolidated that feedback, also helps with DI so this is a good improvement. I agree that splitting by API should be the guideline but not a strict rule. We really do need some standards around it though because we have some wildly different approaches popping up in core already (see file module for example)

Would be good to get this in sooner rather than later so I don't have to rebase it again!

Just to be clear the power api file was meant to make these refactor easier, step three was always going to be logical groupings.

It was also meant to make rebasing easier.

The length of time these are taking is really making me reconsider.

Neither taxonomy nor node are in yet despite fairly active tracking.

I wonder if we need a more coordinated strategy with these.

I wanted to circle back to #17 from @catch:

Yes #10 makes this tricky, we should at least make sure the extra service instantiations aren't in the critical path.

The standout for me is hook_page_top which is invoked everywhere, and now gets 4 services injected. Probably RouteMatch, Renderer and EntityTypeManager would be instantiated anyway, but maybe not FormBuilder. I don't know if this is significant enough to worry about.

I've added a link to #57 on 📌 Explore using more service closures to break deep dependency chains and load fewer services Active , I think we can handle it as a spin-off of that issue rather than this one.

But also I really wondered why that page-specific logic has to be implemented in hook_page_top() at all, so I searched to see if there was an issue about it, and found this one I opened in 2023 :/ 📌 Get rid of node_page_top() Active .

Given there's at least two possible ways we could ensure that form builder doesn't get instantiated on every request by that hook, let's just go ahead here.

fwiw I agree with moving cron/ranking to NodeSearchHooks - looks a lot tidier.

Committed/pushed to 11.x, thanks!

catch → closed merge request !12085

Thanks so much @catch