Problem/Motivation

11.1 excluded hook_preprocess and hook_preprocess_HOOK from OOP and threw an exception when one was added using a Hook attribute.

11.2 adds support for both of these hooks.
We need to remove the logic exception from 11.1 so that contrib can support both. We should trigger a message when the OOP equivalent is used with no corresponding procedural implementation to ensure the hooks run.
https://www.drupal.org/project/drupal/issues/3523109

Steps to reproduce

Proposed resolution

No longer prevent preprocess.
Set error if the procedural hook is missing

Remaining tasks

Update CR with minimum 11.1 version once this is in: https://www.drupal.org/node/3496491

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3524716-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3524716

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.

catch’s picture

catch
he/him
Primary language English
commented

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?

nicxvan opened merge request !12136

nicxvan’s picture

nicxvan commented

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.

Triggered by:
    
    * Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest::testCoreLibraryCompleteness (5 times)
      /builds/issue/drupal-3524716/core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php:115
    
    * Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest::testCoreLibraryCompletenessDeprecated (5 times)
      /builds/issue/drupal-3524716/core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php:167
    
    8) /builds/issue/drupal-3524716/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php:63
    The following theme is missing from the file system: dblog
nicxvan’s picture

nicxvan commented

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?

nicxvan’s picture

nicxvan commented
Status: Active » Needs review

That test is busted on 11.1.x: https://git.drupalcode.org/project/drupal/-/pipelines/499192

nicxvan’s picture

nicxvan commented
Issue summary: View changes

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.
nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

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.

nicxvan’s picture

nicxvan commented
Related issues: +#3525090: [11.1] ResolvedLibraryDefinitionsFilesMatchTest is failing

Created #3525090: [11.1] ResolvedLibraryDefinitionsFilesMatchTest is failing for the failing test on 11.1.x HEAD

nicxvan’s picture

nicxvan commented
Issue summary: View changes
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3524716-nr-bot.txt91 bytes

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.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot

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

nicxvan’s picture

nicxvan commented
Issue tags: +11.2.0 release priority
nicxvan’s picture

nicxvan commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
nicxvan’s picture

nicxvan commented

I updated the CR in https://www.drupal.org/node/3496491

quietone closed merge request !12136

nicxvan’s picture

nicxvan commented
Component: base system » extension system

Status: Fixed » Closed (fixed)

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