Page MenuHomePhabricator

Enforce concept of unabortable hooks
Closed, ResolvedPublic

Description

Developers have been surprised by unexpected aborts from hooks for a long time. After various issues in 2012 and 2013 (such as T51727) Ori made an improvement to the Hooks system:

This made it so that hooks don't need to specify a return value. They can simply leave out the return statement entirely (or use an explicit return without a value for cases where an early return is needed). The hook runner in core for those cases already ignored the return value anyway. The only effect returning early has, is that, other hooks internally queued after the current one, will not run.

However, while a return statement is no longer a requirement, it is still possible to have a return statement. And as long as there is one, or as long as someone might accidentally introduce one, that return statement can be changed to false or a string, and break stuff all over again.

Which, of course, has happened:

The worst part is that these things happen quietly and are very hard to debug because the Hooks system makes the assumption this happened intentionally.

I'm proposing to make another change, which is to add a flag that can be set on Hooks::run that makes a run intended as "non-abortable", meaning any boolean or string return value will be considered an error.

This flag will be opt-in since we do have genuine cases of abortable hooks (although not nearly as many). To ease migration, we can continue to allow explicit return true; even on non-abortable events.

Event Timeline

Krinkle created this task.Aug 19 2017, 12:45 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 19 2017, 12:45 AM

Change 372594 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Hooks: Introduce NO_ABORT mode in Hooks::run()

https://gerrit.wikimedia.org/r/372594

Change 372595 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Mark various skin/OutputPage hooks as unabortable

https://gerrit.wikimedia.org/r/372595

Change 372594 merged by jenkins-bot:
[mediawiki/core@master] Hooks: Introduce Hooks::runWithoutAbort() alongside Hooks::run()

https://gerrit.wikimedia.org/r/372594

Change 372595 merged by jenkins-bot:
[mediawiki/core@master] Mark various skin/OutputPage hooks as unabortable

https://gerrit.wikimedia.org/r/372595

Krinkle closed this task as Resolved.Sep 20 2017, 1:45 PM
Krinkle claimed this task.
Krinkle triaged this task as Normal priority.
hashar added a subscriber: hashar.

May you potentially add a release note for the next 1.31 release? I had at least one extension suddenly failing because of that :]

Maybe the unabortable hook could be documented as such in doc/hooks.txt?

Krinkle added a comment.EditedJun 26 2018, 2:36 PM

@hashar This wasn't a breaking change for the public API.

Normal hooks (abortable or not abortable) support the following return values:

  • null (=true),
  • true,
  • false,
  • string (=like false + error msg).

PCRGUIInserts seems to have code that returns... an OutputPage object! It looks like it was just a mistake that went undetected until now.
https://gerrit.wikimedia.org/g/mediawiki/extensions/PCRGUIInserts/+/3a235fafbd/PCRGUIInserts.class.php#39

Unabortable hooks "should" have no return value (not even boolean). But, for backwards-compatibility, I made unabortable hooks also support return true. Other return values for unabortable hooks are a mistake in the code.

The name "Unabortable hooks" is new, but the idea is not new.

if ( Hooks::run('Example') ) {
 // Abortable hook, the return value is used by the if-statement.
}

Hooks::run( 'Example' );
// Unabortable hook, return value is not used.
// If a handler for this kind of hook tries to return false,
// that is a mistake, because it does not do anything.