Skip to content

Introduce HookEssentials#5474

Merged
lippserd merged 6 commits into
mainfrom
hookEssentials
Jun 23, 2026
Merged

Introduce HookEssentials#5474
lippserd merged 6 commits into
mainfrom
hookEssentials

Conversation

@Al2Klimov

@Al2Klimov Al2Klimov commented Feb 16, 2026

Copy link
Copy Markdown
Member

Introduce Icinga\Application\Hook\HookEssentials as the shared API for hook base classes to register and retrieve hook implementations through their own type.

This moves existing core hook classes to methods such as register(), all(), first(), and isRegistered(), while keeping the underlying hook registry behavior unchanged.

The new trait centralizes common hook behavior and lets hook base classes define their registry name once through getHookName(). It also adds getModule() for hook implementations that belong to a module namespace.

Existing direct uses of Hook::has(), Hook::all(), Hook::first(), and Hook::register() are replaced where a typed hook base class is available. The old low-level APIs and Module::provideHook() are marked as deprecated in favor of Hook\HookEssentials.

@Al2Klimov Al2Klimov requested a review from lippserd February 16, 2026 15:09
@Al2Klimov Al2Klimov self-assigned this Feb 16, 2026
@cla-bot cla-bot Bot added the cla/signed label Feb 16, 2026
@Al2Klimov Al2Klimov marked this pull request as draft February 16, 2026 15:33
@Al2Klimov Al2Klimov marked this pull request as ready for review February 16, 2026 15:45
@Al2Klimov Al2Klimov removed their assignment Feb 16, 2026

@Al2Klimov Al2Klimov left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test

  1. Install Fedora 42
  2. Apply https://github.com/Al2Klimov/ansible-icinga-fedora
  3. Downgrade to https://git.icinga.com/packages/icingaweb/-/jobs/817857
  4. Ensure /icingaweb2/health shows Icinga DB and Redis
  5. In /usr/share/icingaweb2/modules/icingadb/run.php, comment out $this->provideHook('health', 'RedisHealth');
  6. Ensure /icingaweb2/health shows Icinga DB only
  7. In /usr/share/icingaweb2/modules/icingadb/run.php, add \Icinga\Module\Icingadb\ProvidedHook\RedisHealth::register();
  8. Ensure /icingaweb2/health shows Icinga DB and Redis again = it works

@Al2Klimov

Al2Klimov commented Mar 4, 2026

Copy link
Copy Markdown
Member Author

@Al2Klimov Al2Klimov force-pushed the hookEssentials branch 2 times, most recently from 0fd0f9d to 9b20fcd Compare March 4, 2026 14:45
@jrauh01 jrauh01 mentioned this pull request Mar 5, 2026
@Al2Klimov Al2Klimov removed the request for review from lippserd March 26, 2026 13:26
@Al2Klimov Al2Klimov marked this pull request as draft March 26, 2026 13:26
@Al2Klimov Al2Klimov self-assigned this Mar 26, 2026
@Al2Klimov Al2Klimov marked this pull request as ready for review March 26, 2026 14:25

@sukhwinder33445 sukhwinder33445 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase.

Comment thread library/Icinga/Application/Hook/Essentials.php Outdated
Comment thread library/Icinga/Application/Hook/Essentials.php Outdated
Comment thread library/Icinga/Application/Hook/Essentials.php Outdated
Comment thread library/Icinga/Application/Hook/Essentials.php Outdated
@lippserd lippserd self-requested a review April 13, 2026 10:09
@lippserd lippserd changed the title Introduce \Icinga\Application\Hook\Essentials Introduce HookEssentials Jun 22, 2026
@lippserd

Copy link
Copy Markdown
Member

I made the following changes:

  • Renamed Essentials to HookEssentials throughout
  • Replaced the trait-alias pattern in LoginButtonHook and RequestHook (use Essentials { register as protected parentRegister; } + register() override) with an isAlwaysRun(): bool class method override; register() now accepts ?bool $alwaysRun = null and falls back to static::isAlwaysRun()
  • Added getModule(): ?Module, ported from Icinga DB Web's HookUtils, generalized to return null for hooks outside a module namespace
  • Completed two missed migrations: Hook::first('Pdfexport') in Pdf.php and Hook::first('ThemeLoader') in StyleSheet.php

@Al2Klimov please have a look since I have amended some of the changes into your commits.
@TheSyscall please review.

@lippserd lippserd requested a review from TheSyscall June 22, 2026 12:44

@TheSyscall TheSyscall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I searched through the codebase and I can't find any hook that doesn't use the new HookEssentials methods. (At least in icingaweb itself)
  • Tested the functionality behind these hooks, and they seem to work just fine.
  • getModule should probably be final, but that is sadly not possible in PHP.

Finding(s):

getHookName inside the hook implementations should be made final in all instances, overriding it could lead to strange bugs. But that would be entirely self-inflicted.

Comment thread library/Icinga/Application/Hook/HookEssentials.php
TheSyscall
TheSyscall previously approved these changes Jun 23, 2026

@TheSyscall TheSyscall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Al2Klimov and others added 6 commits June 23, 2026 12:11
Co-authored-by: Eric Lippmann <eric.lippmann@icinga.com>
Co-authored-by: Eric Lippmann <eric.lippmann@icinga.com>
Co-authored-by: Eric Lippmann <eric.lippmann@icinga.com>
Co-authored-by: Eric Lippmann <eric.lippmann@icinga.com>
The `use HookEssentials { register as protected parentRegister; }` alias
construct in `LoginButtonHook` and `RequestHook` is harder to read than a
simple method override. A `protected static isAlwaysRun()` default on the
trait lets subclasses declare their intent without redefining `register()`.

A static property was not viable here: PHP forbids redefining a trait
property in a using class with a different initial value.
Ported from `Hook\Common\HookUtils` in Icinga DB Web, generalized to return
null when the hook is not registered under a module namespace.
@lippserd lippserd merged commit fd6a977 into main Jun 23, 2026
13 checks passed
@lippserd lippserd deleted the hookEssentials branch June 23, 2026 10:41
@lippserd lippserd added this to the 2.14.0 milestone Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants