It might be worth waiting on the Async Fragments work as this hook will be replaced as part of that anyway? But good idea, so if this is pressing please go ahead!

In T304238#7814546, @Jdforrester-WMF wrote:

It might be worth waiting on the Async Fragments work as this hook will be replaced as part of that anyway? But good idea, so if this is pressing please go ahead!

Never heard about Async Fragments before :) But I think this is not urgent anyway.

It should have a single, clear-defined way to add errors. Currently, the code which checks the hook result is awful because the result can change depending on whether the hook returned false, whether the Status is OK/good, and whether the status' value is set to something. Also, it shouldn't need to add the 'hookaborted' fallback message.

I have just wasted half an hour looking into that. A status that is OK but not good (like StatusValue::newError('foo')) is treated as a successful merge. Except when the hook returns false, in which case a good status will be turned into a fatal, but a OK but not good status will be left as is (but the edit will still not be done). The status is then passed up the chain, so EditPage::attemptSave() might return an OK status with the edit not done, or McrUndoAction might return a non-good status (which will trigger the error handling branch of the undo form) with the undo action done.

I guess this is more about StatusValue being terrible than about EditFilterMergedContent, but its special-casings don't help.