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.

It is important to keep the issue summary as clear and up-to-date as possible so that people can get involved with the issue at all stages (ideally) and easily understand what it is about. Otherwise you restrict it to people who are 'in the know' ie those who have floated around in the wine vat of the issue for the duration of its maturation.

I think if we operated issues like a production line rather than a brewing process efficiency would improve.

You did add none to remaining tasks:
https://www.drupal.org/node/3502014/revisions/view/13865170/13865177

I removed it in comment 5.

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

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.

Feel free to ping me in slack if you have more questions, I'm happy to chat more, but no need to add to this issue.

@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.

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.

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

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?

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.

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.

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.

No big deal I suppose, but that's how I would have done it.

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.

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

I think this is safe to RTBC again!

Looks really good, thanks!

Mostly documentation nits, but a few param types which would be good to sort out before we commit and ship this.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

I think everything has been addressed again!

I feel like this issue is a good place for me to mention that the core Taxonomy module is seeking a subsystem maintainer! :)

This is going to be a rebase and the deprecation needs to be updated to 11.3.0.

I don't think this counts as a disruptive deprecation so I assume 12.0.0 is fine for removal.

Sorry, should be needs work for the reroll.

I thought this had gotten in!

The bulk conversion got the suggestions, let me rebase this, I think we could get this in.

Rebase was pretty clean, I updated the deprecation version, I don't think those two functions are particularly disruptive so I left them at 12.

build node index is only called in https://www.drupal.org/project/private_taxonomy which has no supported drupal release.

delete node index is called in https://www.drupal.org/project/taxonomy_deep_index which also has no supported drupal release.

mcgovernm has put a ton of effort in, let's get this across the finish line.

Initial pass at saving credit:
@mcgovernm for (almost) all the code
@nicxvan for heavy reviews, the rebases, and some of the code
@berdir and myself for detailed MR reviews

At this point, none of the other commenters here seem to reach the level of "creditable contribution", at least to my eyes.

This is really quite close. Opened some more MR nits. I don't see anything else to improve in here.

Also, edited the CR since this is 11.3 material at this point.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

I've been over the MR multiple times with a fine-toothed comb. All but one of my concerns have been addressed, or at least follow-ups opened. The remaining open thread could also be a follow-up, it's a very minor nit about defgroup and I'm not entirely sure how relevant those tags even are anymore. But I'll leave it open for a committer to review before merging this.

Latest pipeline is mostly green. The Validatable config job is failing, but this MR isn't changing anything about config validation, so I don't think it's worth investigating why that job failed.

I could probably come up with some more hyper-pedantic nits if I really tried, but I'm trying to relax that approach. 😂 Progress is better than stalled perfection, so let's get this in and move on.

Thanks!
-Derek

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.