I’ve had this for quite some time, just never got around committing it:

diff --git a/src/HookHandler.php b/src/HookHandler.php
index aa6824dfc..6b6cff4df 100644
--- a/src/HookHandler.php
+++ b/src/HookHandler.php
@@ -861,7 +861,7 @@ class HookHandler implements
                string $summary,
                User $user
        ): bool {
-               if ( !$content instanceof TextContent ) {
+               if ( !$content instanceof TextContent || $context->getTitle() === null ) { // TODO causes CI failure in core, isn’t a problem in prod?
                        // Not interested
                        return true;
                }

I still don’t know if it’s a potential problem in prod, but maybe I should commit it just for the sake of fixing CI?

On the other hand, having an edit being made to no page makes no sense (and indeed, the test creates an Article with a specified title, it just doesn’t set the same title on the context). Maybe the hook should be changed (i.e. deprecated and replaced with another hook) to always provide the title that McrUndoAction has? (If we go that way, I’d like to hear the opinion of @SomeRandomDeveloper to avoid choosing a direction that conflicts with the EditPage refactoring.)

Change #1336624 had a related patch set uploaded (by CoderAnimeshPathak; author: CoderAnimeshPathak):

[mediawiki/extensions/Translate@master] Fix TypeError in HookHandler when context title is null

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

In T437107#12292123, @Tacsipacsi wrote:

[...] Maybe the hook should be changed (i.e. deprecated and replaced with another hook) to always provide the title that McrUndoAction has? (If we go that way, I’d like to hear the opinion of @SomeRandomDeveloper to avoid choosing a direction that conflicts with the EditPage refactoring.)

I agree that EditFilterMergedContent should be replaced at some point, e.g. to replace the Status with a StatusValue, User with UserIdentity, and maybe no longer provide a full IContextSource as callers shouldn't depend on the request anymore, but instead a PageIdentity/ProperPageIdentity for the page that's being edited. We should also consider passing more data about the edit to the hook (maybe an instance of a new class which holds some data about the edit, e.g. the summary, whether it is a minor edit, etc so we can provide more or less data in the future without replacing the hook signature again). I haven't reviewed the callers yet, but I think it's likely that some depend on request values right now.
Replacing hooks that still use a full EditPage instance (T251588) currently has a higher priority for me, but feel free to upload a patch if you or anybody else wants to work on replacing EditFilterMergedContent.

Thanks for your response! I didn’t mean to ask you to prioritize this, I just wanted to avoid us taking a direction that would make further refactoring of EditPage more difficult (or would result in yet another deprecation). Introducing a new class to store the parameters is a good idea, I also thought about it.

Another idea: what about scratching EditFilterMergedContentHookConstraint, and instead letting extensions provide EditConstraints (of course once EditConstraint is no longer @internal)? For example:

class PageEditGetConstraintsInputs {
	public function __construct(
		public readonly IContextSource $context, // escape hatch as there are always going to be extensions that need it (e.g. ConfirmEdit currently uses it to access the request), but should have a warning to avoid using it if possible
		public readonly PageEditInputs $inputs, // this stores the edit summary, the page and the user, maybe it could also store the minor edit state?
		public readonly Content $content,
		public readonly bool $minorEdit,
	) {
	}
}

class PageEditGetConstraintsHook {
	/**
	 * @param PageEditGetConstraintsInputs $inputs
	 * @param EditConstraint[] &$constraints Array to add constraints to
	 * @return void No return value, the hook must not abort
	 */
	public function onPageEditGetConstraints( PageEditGetConstraintsInputs $inputs, array &$constraints ): void;
}

In T437107#12292644, @Tacsipacsi wrote:

Another idea: what about scratching EditFilterMergedContentHookConstraint, and instead letting extensions provide EditConstraints (of course once EditConstraint is no longer @internal)? For example:

class PageEditGetConstraintsInputs {
	public function __construct(
		public readonly IContextSource $context, // escape hatch as there are always going to be extensions that need it (e.g. ConfirmEdit currently uses it to access the request), but should have a warning to avoid using it if possible
		public readonly PageEditInputs $inputs, // this stores the edit summary, the page and the user, maybe it could also store the minor edit state?
		public readonly Content $content,
		public readonly bool $minorEdit,
	) {
	}
}

class PageEditGetConstraintsHook {
	/**
	 * @param PageEditGetConstraintsInputs $inputs
	 * @param EditConstraint[] &$constraints Array to add constraints to
	 * @return void No return value, the hook must not abort
	 */
	public function onPageEditGetConstraints( PageEditGetConstraintsInputs $inputs, array &$constraints ): void;
}

I also had that idea at some point a couple months ago, but nowadays I don't really like it anymore after putting some more thought into it, because an edit constraint would just be a hook handler with extra steps, right?
EditConstraint only has two methods, getName() (which is only relevant for logging) and checkConstraint(), which works similar to a hook handler. I don't really see a point in using edit constraints instead of a hook (or multiple hooks if necessary, e.g. for checks that are run during preview or earlier during an edit, like the preliminary checks).

The constraints-based approach has three advantages over a simple hook:

• The returned value is in the return value rather than in a mutable parameter. (Although the mutable parameter comes back in onPageEditGetConstraints().)
• There is only one returned value: the PageEditStatus. An EditConstraint has no way to “abort”, which is currently another way to signal a failure; and the registering hook has no reason to abort, so it can be made unabortable. (Although we could also say that the replacement hook, which just changes the parameter types but is still called by EditFilterMergedContentHookConstraint, is unabortable.)
• You say getName() is only relevant for logging. However, it is relevant for logging, and if we use separate EditConstraints, that logged name would become more useful even if the method is not overridden (compare Check for EditFilterMergedContentHookConstraint failed with Check for TranslateValidateMessage failed). The hooks system has AFAIK no way to find out what piece of code put the warning or error message in the StatusValue.
• (And a fourth advantage: easier to find a new name to replace the to-be-deprecated EditFilterMergedContent. ☺)

Another option would be using the middleware pattern, like in NotificationsMiddleware, but that feels like a one-off solution rather than a well-established pattern within MediaWiki (e.g. the extension.json key is NotificationsMiddleware rather than having a generic key like Middlewares with a subkey Notifications). Although if you want to establish the middleware pattern in MediaWiki, do go for it!

On the topic of replacing EditFilterMergedContent: T304238: Replace the EditFilterMergedContent hook with a more modern one.

Change #1336624 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] Fix TypeError in HookHandler when context title is null

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

RoadToWiki board hygiene: moved this task to Completed so the board matches Phabricator status (resolved/closed). No change to task status or assignee.