Clean up hook implementations in the Taxonomy module

@oily, please make sure when you're editing the issue summary you're reviewing what is actually needed, there are definitely remaining tasks for this issue, they are just not added yet.

@mcgovernm this is a great first step!

I think there are a couple of things we want to do in these that we wouldn't normally do for organization since they are hooks.

1. Add the appropriate return types for the methods if they do not exist.
2. Add dependency injection.

These are autowired, in case you are not familiar with in general it means you should be able to just add a use statement for the interface of the service you need, then add that service to the constructor.

Let me know if you have any questions!

I'll take a deeper look later.

@nicxvan wrt #5

@oily, please make sure when you're editing the issue summary you're reviewing what is actually needed, there are definitely remaining tasks for this issue, they are just not added yet.

I did not edit 'Remaining tasks'. It is blank. If you are referring to the other fields, I filled them based on what I thought appropriate. 'None' could be thought of as 'None' ever or 'None' just now.

You did add none to remaining tasks, I removed it in comment 5.

You did the same in this issue here too: 📌 Drupal\Core\Theme\ComponentNegotiator::negotiate uses a lot of memory Active

I agree keeping issue summaries up to date is useful, I'm not trying to discourage you from updating them.

I am trying to encourage you to review the actual issue and provide relevant updates.

There are remaining tasks both in this issue and the other, no need to write them or if you don't know what they are, but none is not accurate.

@nicxvan RE: #10 I did not realise you had had to redo. I dont want to create work for others. So I agree, I will be more circumspect about completing issue summaries.

taxonomy_theme_suggestions_taxonomy_term can be manually converted, the rector rule missed all theme suggestions.

Tests started failing after adding the return type to hook_help in this commit. I have not been able to figure out why this would change anything.

Time: 00:37.972, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest::testLogin
Failed asserting that 61 is identical to 60.

/var/www/html/core/modules/navigation/tests/src/FunctionalJavascript/PerformanceTest.php:75

Line 75 of PerformanceTest.php:
$this->assertSame(60, $performance_data->getCacheGetCount());

I wouldn't worry about it, unless you think this is ready for review at the moment. It's most likely an upstream change affecting that and will be fixed separately.

When I see these I usually try to track down the issue that introduced it, and open an issue to resolve it, but that's not strictly necessary if you're still working on other feedback on the issue.

mcgovernm → changed the visibility of the branch drupal-3502014-3502014-clean-up-hook to hidden.

Putting back to needs work, as taxonomy_theme_suggestions_taxonomy_term still needs to be addressed.

Sorry which branch? Can you hide the ones you're not using?

mcgovernm → changed the visibility of the branch 3502014-clean-up-hook to hidden.

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

Looks great!

Couple of comments, I didn't provide a lot of detail feel free to ping me on slack if you need clarification.

I think this needs a clean rebase.

Sorry I was looking at the old MR, is this ready for review again?

Thanks! This is ready for review again. I had moved the setting of $config to a helper function, but wasn't sure after the last couple of comments if that was necessary or if it could have been left in the constructor for this case.

Helper is good!

Haven't reviewed because @nicxvan seems to be on it but love the file structure picked here.

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.

Thanks for the feedback! I think it was just two classes that needed to be renamed based on the text file, this is ready for review.

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

Great thanks!

getConfig() is still broken. It does not return a Config object, it returns the ConfigFactory because the condition there is never TRUE to go inside the if condition.

It works because it does not have a return type, which I'm not sure why phpstan doesn't complain a about.

The tests pass because what actually happens is that it returns a new config object for the non-existing maintain_index_table config and that evaluates to true. We have no tests that disable this flag.

drop the if, add an explicit return type, and just make it a one-liner to return that config. Since that's the only config key we care about, I'd actually suggest to rename the method to something like shouldMaintainIndexTable() and directly return a boolean from that.

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

Thank you @berdir! I've just pushed a commit to address that. I also noticed we still need to remove taxonomy_build_node_index and taxonomy_delete_node_index from taxonomy.module since they've been moved to TaxonomyEntityHooks. I should be able to get to that shortly.

Unsure about the helper functions. Unlike some others, they are not underscore-prefixed nor are they hooks, so technically they are an API. in https://git.drupalcode.org/project/drupal/-/merge_requests/10999#note_45... I proposed to keep them but deprecate them for D12.

Yeah those need to be deprecated.

Two minor comments! Getting pretty close again

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

FWIW, I specifically suggested to not introduce a helper service for the node indexing and keep them as protected functions. It's not an API that taxonomy module offers, so it makes more sense to me as protected methods. Yes, the logic is duplicated then on the functions, but I don't think there is a rule against that.

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

In https://git.drupalcode.org/project/drupal/-/merge_requests/10999#note_45..., yes.

Thank you @berdir. I've created a new branch and on it rolled back the commit that moved it to a service. New draft MR is here. Hopefully I've deprecated these correctly, I assume we'll need to update the CR to indicate these have been moved to protected methods?

Diff between new and current MR

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

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