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 :)