See this change record: Core added support for OOP hook implementations, support for procedural hooks (with magic names) is expected to be phased out mid-term. There is already a proposed way of making this switch without depending on Drupal 11.1 so we can implement this any time we want.

This will be especially nice for us as we are currently mixing different concerns, namely framework code plus code for specific plugins, in hook implementations. Now, we can just have two implementations.
As most of the actual code for hook implementations was already moved to classes, it also seems like we might just need to move them to the new location (\Drupal\search_api\Hook) – keeping BC in mind, of course.

Issue fork search_api-3489482

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

drunken monkey created an issue. See original summary.

sayan_k_dutta’s picture

sayan_k_dutta commented
Assigned: Unassigned » sayan_k_dutta

Working on it.

sayan_k_dutta opened merge request !217

sayan_k_dutta closed merge request !217

sayan_k_dutta opened merge request !218

sayan_k_dutta changed the visibility of the branch 3489482-switch-to-oop to hidden.

sayan_k_dutta changed the visibility of the branch 3489482-switch-to-oop to active.

sayan_k_dutta’s picture

sayan_k_dutta commented

Working further to convert the views_hooks in oop implementation.

sayan_k_dutta’s picture

sayan_k_dutta commented
Assigned: sayan_k_dutta » Unassigned
Status: Active » Needs review

Made the changes. Please review MR !218

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Needs work

Thanks a lot for your work on this, great job so far! However, there are still a couple of problems, as evidenced by the failing tests:

  • I think SearchApiViewHooks is missing a services.yml entry.
  • (I’d also call that class SearchApiViewsHooks, as the module name is “Views”.)
  • I don’t think we can use autowire yet, but I just temporarily enabled testing against other Drupal versions in the fork to validate that assumption.
  • Services in the two Hooks classes should be accessed via dependency injection (which is also why autowire would even be needed, I think), not via the global \Drupal class. As a rule, classes that are able to use dependency injection should not contain the string '\Drupal::'.

I already implemented the first two items, but please address the last one as well as the failing MR pipelines.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
Issue tags: +Vienna2025

Was quite a bit of work to add proper DI and get everything properly organized but I think this is now ready to be reviewed.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Related issues: +#3553016: Fix pipelines for Drupal 10 and Drupal 11.3.x

I think the pipelines should pass now except for “next minor” which currently fails in HEAD as well, see #3553016: Fix pipelines for Drupal 10 and Drupal 11.3.x.
Let’s see.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Merged.
Thanks a lot again for your help on this!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.