Problem/Motivation

Let's document that module_implements_alter and cache_flush cannot be OOP and add to denylist.

CR has been updated: https://www.drupal.org/node/3442349

Steps to reproduce

Proposed resolution

Exclude it from being oop by adding it to procedural only.
Add to #3442009: OOP hooks using attributes and event dispatcher CR

Remaining tasks

N/A

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3483899

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

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Related issues: +#3472214: [META] Convert Core hook implementations to Class or method based implementations

nicxvan opened merge request !9964

nicxvan’s picture

nicxvan commented

Note to check for getImplementationInfo

nicxvan’s picture

nicxvan commented

These can be converted manually using hook order I think. This likely needs documentation.

nicxvan’s picture

nicxvan commented
Title: module_implements_alter is not supported by OOP hooks » Document how to convert module_implements_alter
nicxvan’s picture

nicxvan commented
Title: Document how to convert module_implements_alter » Add hook_module_implements_alter to procedural only
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3484754: Ordering of hooks needs some attention
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: Add hook_module_implements_alter to procedural only » hook_module_implements_alter hook_cache_flush can only be procedural
Related issues: +#3484747: Fix invocation of hook_cache_flush in ModuleInstaller uninstall to allow OOP implementations

nicxvan changed the visibility of the branch 3483899-moduleimplementsalter-is-not to hidden.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !10029

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

This looks fine, so RTBC.

I do have two small out of scope questions:

  • I don't see any test coverage for the existing deny list functionality. If there is actually isn't any, was this an intentional choice as something to be done later?
  • The docblock return type hint for HookCollectorPass::collectAllHookImplementations(): @return \Drupal\Core\Extension\HookCollectorPass has the wrong namespace. It probably should be @return $this. Is it worth worth its own issue?
nicxvan’s picture

nicxvan commented

Good catch, I can create follow ups for those.

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

This looks good (and those two separate follow-ups sound good as well).

Committed/pushed to 11.x/11.1.x, thanks!

  • catch committed a692feee on 11.1.x 
    Issue #3483899 by nicxvan, godotislate: hook_module_implements_alter...

  • catch committed 49f6e6bb on 11.x 
    Issue #3483899 by nicxvan, godotislate: hook_module_implements_alter...
godotislate’s picture

godotislate
he/him
commented

This was moved to Fixed in#19 , but somehow it's still showing as RTBC?

nicxvan’s picture

nicxvan commented
Related issues: +#3485117: Fix return type on HookCollectorPass::collectAllHookImplementations(), +#3485118: Add tests for HookCollectorPass deny list

Weird on status, I created the followups though.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Sometimes that happens if the commit bot posts at the same time as the status was changed. If you see it happen feel free to re-save the issue status to what it was meant to be.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Though credits were lost too, fixing those here.

Status: Fixed » Closed (fixed)

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

nicxvan closed merge request !10029

nicxvan closed merge request !9964