🇺🇸United States @nicxvan

I can update the crs to be more explicit.

https://www.drupal.org/node/3499788 → refers to both issues because neither have landed, whichever lands first the other reference will be removed.

I'll update the crs shortly.

Rebased.

@reinimax did you test this patch?

There is a functional JS failure that looks relevant, I can't rerun it since a core committer created the branch.

I think this might be ready for review again!

The actual proposal did not change. The underlying implementation changed.

If you reviewed this before the changes are in HookCollectorPass, ModuleHandler and the other classes in core/lib/Drupal/Core/Hook
There are many more tests too.

nicxvan → created an issue.

Thanks! Glad I could help!

I checked the merge and saw no changes to the hooks, I pushed a fix for the sniff errors.

I updated the CR and hid the other approach, I think this looks good but I want to give @berdir a chance to look.

nicxvan → changed the visibility of the branch 3502014-EntityHooks to hidden.

Did you mean in a comment on the MR? I don't see that recommendation in these comments.

Made suggestions for all of the things that need tweaking I think.

Should it be moved to core?
Like this: https://www.drupal.org/project/drupal/issues/3491129 📌 Drupal Core strategy for 2025-2028 Active

Two minor comments! Getting pretty close again

nicxvan → changed the visibility of the branch 3240266-varnish-image-purge to hidden.

nicxvan → made their first commit to this issue’s fork.

I've pushed up the class override, I'm not actively working on this at the moment so no need to assign myself.

This needs tests.

Not sure this should be closed, just because there is some movement towards this goal doesn't mean it's been an explicit decision like this issue is looking for.

Yes let's please postpone this

Just to add to it, here is the code we used for benchmarking:

  $r = new \ReflectionClass($object);
  $time = microtime(TRUE);
  $n = count($r->getMethods());
  print (microtime(TRUE) - $time) . \PHP_EOL;

  $time = microtime(TRUE);
  for ($i = 0; $i < $n; $i++) {
    new stdClass;
  }
  print (microtime(TRUE) - $time) . \PHP_EOL;
  print ($n) . \PHP_EOL . \PHP_EOL;

We ran it with different classes with different numbers of methods.
The numbers returned are the time to check the class, the time to create the new stdClass and the number of methods:

0.24680495262146
0.058710098266602
1000000

0.015760898590088
0.0052649974822998
100000

0.0014050006866455
0.00056791305541992
10000

0.00011301040649414
7.2002410888672E-5
1000

1.5974044799805E-5
1.0013580322266E-5
100

2.8610229492188E-6
2.1457672119141E-6
10

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

Ok I've officially updated the IS and CRs in order to get behind the new approach, we need to polish the approach.

nicxvan → changed the visibility of the branch 3485896-attempt-simplification to hidden.

This could use a rebase, we're moving forward with not using template in the preprocess issue, so I'm not sure if that means we can use core here or not.

Personally I'm ok with it.

Yeah those need to be deprecated.

Can you list your module's?

Oh I like that! It let's you define, gives the dev a clear notice but they can do what they choose at that point!

Oh great catch, yes we need to do that too.

I think that covers my concerns!

Great thanks!

Two minor comments you can merge in then I think it's ready!

I think this is closer, I hate to say it, I think we came up with a decent rubric for the split in the meta: #3493453-25: [meta] Standardize and clean up hook classes in core →

I know you've been putting a lot towards this, but can we split the files based on the new thoughts I linked? Once that is in I think it's ready for a final review.

This could probably use a test.

Thank you! I don't quite see the benefit of tugboat for this project, it needs so many things to run including drush and there are no visual elements, am I missing something?

Any chance you can solve those failures?

Thanks everyone, this is on my list!

Ok, we deprecated template_preprocess so it is ok to deprecate here too I think for consistency.

I reviewed this very carefully to ensure everything aligns. I have also reviewed the general functionality several times as you've iterated.

This approach is great!

The only thing I questioned was whether to add any additional comments on theme.api.php

Specifically to explain the static::class shortcut since hooks have to be services, but that might be more confusing since the CallableResolver documentation is available. I lean towards your changes are sufficient.

Also unsure if I need to wait to hear back from other framework managers for #17.

I think this is ready!

Should be pretty straight forward to add a test like in the issue that brought you here.

I don't see an obvious reason not to do this.

Whoops, I must have had an old pipeline run open cause it was green.

Just gonna set to needs review this time.

It was a clean rebase, just new 80 character limit for FormAlter which was missed in the earlier rebase.

Basically, the bot runs checks using the new rules, this needs to be rebased to see the new rules.

Created the follow ups, I think it's safe to RTBC based on that.

nicxvan → created an issue.

nicxvan → created an issue.

smustgrave → credited nicxvan → .

Adding tag back in that case.

nicxvan → created an issue.

I am still working through this.

I'd vote remove.

I would advocate for closing this, 1-4 micro seconds isn't worth chasing. There are far larger bottlenecks.

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.

We definitely want to keep the module name as part of it, finding the right EntityHooks file out of dozens in a project will be a massive pain.

I think general hooks were just ones that were hard to find a place for, the file I attached I think solves that issue though since we can just use where the hooks are defined as our guide.

CR looks much better!

I added a couple of questions on the MR.

I'm wondering at this point if we need to wait for the go ahead, this is looking pretty close from my perspective.

nicxvan → created an issue.

@dcam and @acbramley, I agree with both of you.

I think that organization makes sense longer term specifically for contrib and custom code. We also need something better defined so we can make progress in core. I'm going to update the issue summary, but I think we follow three steps for organizing / converting 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 have attached a file with the core hooks and the recommended grouping.

For ease of searching I've also created a text file I have attached so you can search for the hook class the hook implementations should go in.

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

I think this approach makes sense, we are going to have to break this up piece by piece

There are some suspicious failures though.

I think we should do the minimal like you did here then work on organization in follow ups.

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

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.

Helper is good!

I am still working on this.

I created a new branch for you to use, please use the issue instructions to pull down the drush12 branch and push there.
You can create an MR from that once you're ready and set it to needs review.

nicxvan → changed the visibility of the branch 1.0.x to hidden.

nicxvan → changed the visibility of the branch 3472566-unable-to-disable to hidden.

Can you put that on an MR so I can test it? Drush changed how commands are registered between 12 and 13 as far as I know.

If you're still on drush 12 you must be on drupal 10 right? The initial alpha 1 version should still work for you.

nicxvan → created an issue. See original summary → .

nicxvan → created an issue.

nicxvan → created an issue.

nicxvan → created an issue.

I don't know that matkconroy is in favor, I think he created this to track catch's discussion, but I don't want to speak for him.

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.

Ok I looked through your changes, they all make sense to me.

As for the conversion, I think this needs to be done in a couple of stages, I think what you have converted here is great, we probably want to follow a similar tactic to what we did for hooks, let's convert enough in this issue to prove the concept, which I think you've done.

Once this is in we can complete 📌 [pp-1] Convert Template Preprocess hooks to OOP equivalent Postponed: needs info to convert the rest and deprecate, then we can discuss better organization ad nauseam in follow ups. The thing is since these are in initial preprocess changing the locations for organization won't affect reordering.

This should be ready now.

I figured out why I needed the empty baseline.

It was because during the conversion there were thousands of changes. A normal baseline process doesn't need that.

nicxvan → changed the visibility of the branch 3485896-donquixote-order-at-call-time-nicxvan-changes-BAD to hidden.