[11.1] update gather rules to manage hook preprocess

Created on 15 May 2025, 8 days ago

Problem/Motivation

We need to update 11.x so that it collects preprocess without exception but warn that legacy hook is required.
https://www.drupal.org/project/drupal/issues/3523109 πŸ“Œ Rethink #[Hook] attribute inheritance Active

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.1 πŸ”₯

Component

base system

Created by

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @nicxvan
  • πŸ‡¬πŸ‡§United Kingdom catch

    One possible idea, it's quite horrible.

    Instead of the exception, can we reverse engineer what the expected function name would be from the OOP hook implementation, check function_exists(), and if it's not there, trigger_error() something?

  • Merge request !12136Move preprocess exception β†’ (Closed) created by nicxvan
  • Pipeline finished with Failed
    8 days ago
    Total: 6758s
    #498415
  • Pipeline finished with Failed
    8 days ago
    #498491
  • Pipeline finished with Failed
    6 days ago
    Total: 751s
    #499440
  • Pipeline finished with Failed
    6 days ago
    Total: 779s
    #499448
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I'm not sure why this is failling:

    core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php fails cause it's looking for modules as themes and they don't exist.

    This test doesn't run locally so I can't easily debug.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    For some reason this change is triggering module handler to not have the modules when LibraryDiscoveryParser::buildByExtension is checking so it checks for modules as themes.

    This doesn't make sense, this shouldn't change anything related to this.

    I even tried an alternate version where we pass the module in through collect by attribute to procedural check and throw the error only there if the function doesn't exist.

    I reverted my changes and it still fails. Is this an 11.1.x only failure?

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I added a test for this specifically, but even with the expect exception for the one we expect, I'm getting a symfony exception.

    I could use some assistance getting past this:

    1) Drupal\KernelTests\Core\Hook\HookCollectorPassTest::testHookPreprocessNoFunction
    Symfony\Component\HttpFoundation\Exception\SessionNotFoundException: There is currently no session available.
  • Pipeline finished with Failed
    6 days ago
    Total: 138s
    #499464
  • Pipeline finished with Failed
    6 days ago
    Total: 748s
    #499468
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I converted to trigger_error as suggested in 2. There doesn't seem to be a way to assert user warnings anymore. I tried converting to an exception in the test as suggested by the phpunit issue, but I run into the same symfony exception.

  • Pipeline finished with Failed
    6 days ago
    Total: 940s
    #499472
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    This is truly ready for review now!

    Please credit @fathershawn for his help on slack getting me past the test issues!

    The failure is unrelate I think and exists on 11.1.x HEAD.

  • Pipeline finished with Failed
    6 days ago
    Total: 527s
    #499725
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Created πŸ› [11.1] ResolvedLibraryDefinitionsFilesMatchTest is failing Active for the failing test on 11.1.x HEAD

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • 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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think the bot is wrong here cause it's a different branch.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Success
    3 days ago
    Total: 651s
    #501387
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • πŸ‡¬πŸ‡§United Kingdom catch

    This looks great.

    Currently, if you add an OOP preprocess hook, you get an exception on 11.1 because it doesn't support preprocess has OOP hooks, and it's trying to stop you prematurely converting things that won't work.

    With the MR, the next 11.1 patch release will allow the OOP hook to exist and ignore it, as long as there's a procedural version too - e.g. still using LegacyHook, but warn you if the legacy hook is missing.

    The result is that modules will be able to require >= 10.4.0 | >= 11.1.8 (or whatever exact version it ends up in) if they want to convert. Otherwise their only option would be >= 10.4.0 | > 11.2.0

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Committed 59f1bee and pushed to 11.1.x. Thanks!

    • alexpott β†’ committed 59f1bee0 on 11.1.x
      Issue #3524716 by nicxvan, catch: [11.1] update gather rules to manage...
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
Production build 0.71.5 2024