Change 660311 had a related patch set uploaded (by Gerrit Patch Uploader; owner: Func):
[mediawiki/core@master] Set $status->value in EditFilterMergedContentHookConstraint::checkConstraint() properly to display error message

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

Can you explain what are the consequences of this change? What extensions are affected?

In T273354#6805178, @matmarex wrote:

Can you explain what are the consequences of this change? What extensions are affected?

Nothing will be affected. Just avoid to set $status->value manually when the handler in the extension rerurning false after $status->fatal().

And maybe use $status->fatal() without returning false in this hook should be deprecated? There is a bug with this undocumented usage: T271037

After looking into it, I don't think the proposed change is correct… I believe that with the AS_HOOK_ERROR status, it's expected that your extension has already displayed an error message on its own. Changing it to AS_HOOK_ERROR_EXPECTED could break compatibility.

In T273354#6820315, @matmarex wrote:

After looking into it, I don't think the proposed change is correct… I believe that with the AS_HOOK_ERROR status, it's expected that your extension has already displayed an error message on its own. Changing it to AS_HOOK_ERROR_EXPECTED could break compatibility.

How can extension display an error message on its own? Extensions can't use $this->hookError, right?

Acording to codesearch, up to now, NO extension returning false will "display an error message on its own", so user can see nothing about their mistake.
See:

1. TranslateHooks.php#1153
2. NewsletterHooks.php#359
3. GadgetHooks.php#329
4. AkismetKlik.php#39

Even worse, with the AS_HOOK_ERROR status, users who hit this hook will probably lose their works, because the edit form wouldn't show up.
But in the document, it says,

returns false means "don't save but continue user interaction", e.g. show the edit form.

So I think the original design is problematic.

Sorry for late response, I had to work out the details.

In T273354#6821414, @Func wrote:

In T273354#6820315, @matmarex wrote:

After looking into it, I don't think the proposed change is correct… I believe that with the AS_HOOK_ERROR status, it's expected that your extension has already displayed an error message on its own. Changing it to AS_HOOK_ERROR_EXPECTED could break compatibility.

How can extension display an error message on its own? Extensions can't use $this->hookError, right?

Use the given $context object to directly display the message and the edit form. Roughly like this (but please, nobody use this code in their extension, it's a bad idea for several reasons):

$wgHooks['EditFilterMergedContent'][] = function (
	IContextSource $context, Content $content, Status $status,
	$summary, User $user, $minoredit
) {
	if ( $content instanceof WikitextContent && strpos( $content->getText(), 'hello' ) !== false ) {
		$context->getOutput()->addElement( 'div', [ 'class' => 'errorbox' ], 'HELLO ERROR' );
		
		$article = Article::newFromTitle( $context->getTitle(), $context );
		$editor = new EditPage( $article );
		$editor->setContextTitle( $context->getTitle() );
		$editor->importFormData( $context->getRequest() );
		$editor->showEditForm();
		
		return false;
	}
};

The above code, together with your change, causes two edit forms to appear when the error is shown.

The 'EditFilterMergedContent' hook evolved from the 'EditFilterMerged' hook, removed in MW 1.29.0 (https://www.mediawiki.org/wiki/Manual:Hooks/EditFilterMerged). That hook received the EditPage object as a parameter, so this pattern was more natural (and didn't require constructing an EditPage object, which gives me heebie-jeebies). Back in the day, you'd use that hook kind of like this: [untested code]

$wgHooks['EditFilterMerged'][] = function (
	EditPage $editor, $text, &$error, $summary
) {
	global $wgOutput;
	if ( strpos( $text, 'hello' ) !== false ) {
		$wgOutput->addElement( 'div', [ 'class' => 'errorbox' ], 'HELLO ERROR' );
		$editor->showEditForm();
		return false;
	}
};

The code comments in EditFilterMergedContentHookConstraint.php mention the ConfirmEdit extension, so I went looking in the history to see how it used that hook (the current code uses AS_HOOK_ERROR_EXPECTED), and I found this change from 2014 replacing the old hook: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ConfirmEdit/+/177746/2/Captcha.php#b505

With the explanation out of the way… I actually agree with you now that this is problematic, and given how difficult it is to display the error message this way, I'll make a guess that this was not intended, and was just copy-pasted from the old hook's code without thinking about it too hard.

I think it would be okay to change it, but we should mention the change to the hook's behavior in RELEASE-NOTES-1.36 under "Breaking changes".

I'd also like someone else to double-check this. @Daimona? @DannyS712?

@matmarex Thanks! And would you like help to check the problem below?

In T273354#6806753, @Func wrote:

And maybe use $status->fatal() without returning false in this hook should be deprecated? There is a bug with this undocumented usage: T271037

Change 660311 merged by jenkins-bot:

[mediawiki/core@master] Set $status->value in EditFilterMergedContentHookConstraint::checkConstraint() properly to display error message

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

Some extensions running this hook (not using it) - https://codesearch.wmcloud.org/search/?q=%27EditFilterMergedContent%27&i=nope&files=&excludeFiles=&repos=

Not sure if that needs adjust as well.

In Change 680687, @Umherirrender wrote:
Returning false with that value acts the same as returning true with a fatal Status?

Why this change? What does this change fix? Is there an error with the hook to fix?

If this is for the deprecation of the fatal Status usage here, that should be done with a deprecation cycles.

@Umherirrender, I noticed that you commented on Change 680687 before here (in the order of emails)?

1. Not only "that value", but also any customized status value can work to display error message properly on mediawiki version 1.36 and before. Not required to set the value now, just for backward compatibility.
2. Changes related to T280312 are aim to display error message to user properly, maybe I didn't explain it clearly in commit message.
3. These changes could fix T271037 indirectly though, but I think the core should be fixed too.
4. I think the deprecation cycles is too tough for me, I just mentioned it once above but get no reply (T273354#6806753).

Change 680695 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] Adjust the description of EditFilterMergedContent hook

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

In T273354#7011442, @Func wrote:

In Change 680687, @Umherirrender wrote:
Returning false with that value acts the same as returning true with a fatal Status?

Why this change? What does this change fix? Is there an error with the hook to fix?

If this is for the deprecation of the fatal Status usage here, that should be done with a deprecation cycles.

@Umherirrender, I noticed that you commented on Change 680687 before here (in the order of emails)?

1. Not only "that value", but also any customized status value can work to display error message properly on mediawiki version 1.36 and before. Not required to set the value now, just for backward compatibility.
2. Changes related to T280312 are aim to display error message to user properly, maybe I didn't explain it clearly in commit message.
3. These changes could fix T271037 indirectly though, but I think the core should be fixed too.
4. I think the deprecation cycles is too tough for me, I just mentioned it once above but get no reply (T273354#6806753).

For 2. - the documentation of the hooks says that the error message is shown when a fatal status is returned with the true. Is that not working any more or buggy to need to fixed here? You have just created a sub task and used the same text there and in the commit message, which does not help me to understand the underlying problem. It seems the description here and in the fixed bug is to technical and does not has information of the impact of the bug/feature/task etc.
For 3. - you should not mention or write about non-public security bugs in public tasks like this on

IIRC Translate uses EditFilterMergedContent to display a message to the user because PageContentSave doesn't display anything besides "edit conflict", or at least that's how it used to work when this code was written.

Change 680695 merged by jenkins-bot:

[mediawiki/core@master] Adjust the description of EditFilterMergedContent hook

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