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:
- T52134: Hook functions should not be required to return a value
- rMW5ff7e6fb7a36: Allow explicit & implicit null returns from hook handlers
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:
- (2015) T107980 rEEDUfa4f454bdbf3: Never unconditionally 'return false' from interface hooks
- (2017) T173411 rEPOP55f075bf1132: Remove aborting of BeforePageDisplay hook
- .. and others.
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.