My own perspective is that the “status” system generally works pretty well, and I’d like to use it more. (The main alternative, I suppose, is exceptions, and they have some shortcomings – particularly “checked” exceptions. Sometimes exceptions work, but sometimes status works better.) I also want to be able to have custom subclasses that add strongly typed ways to create a status with a certain value, update its value, and/or get its value – such as PageUpdateStatus with its methods newEmpty( bool $newPage ), setNewRevision( RevisionRecord $rev ), and getNewRevision(): ?RevisionRecord. In Introduce Status subclasses for EditEntity in Wikibase, I found that such methods can make it much nicer to work with the status (see e.g. SpecialMergeItems.php).

If we want to discourage the Status class and encourage the StatusValue class, then we have to deal with the fact that a lot of existing code still deals with Status, and some code uses Status::wrap(); if a custom StatusValue subclass is wrapped in this way, it will be turned into a generic Status. This is fine as long as the entire state of the status is captured in the fields that Status::wrap() copies over, particularly its $value; so my suggestion would be that, if you make any custom status subclass, you must not introduce any additional properties, but must instead use the $value. This means that PageUpdateStatus is fine as it currently stands, whereas PermissionStatus would have to be changed. (As far as I can tell, PermissionStatus doesn’t actually use the $value field at all, so it should be possible to move the $block, $rateLimitExceeded and $permission in there as an array, just like in PageUpdateStatus.) And this way, if you have a custom status subclass that gets passed through a layer in MediaWiki which turns it into a generic Status (like HTMLForm’s submit callback), you can just turn it back into your custom status subclass, without losing any information.

For my purposes, I would also be fine with additional restrictions like “don’t use $successCount and $failCount” or “don’t use $statusData”, if that’s seen as helpful to not hinder future evolution of the status classes, and unblocks creating custom subclasses today.

On your three questions:

• My vague understanding is that we were trying to discourage Status and move all things to use StatusValue, but having said that I can't find it written down anywhere clearly from some quick poking.
• I think wrapping a StatusValue, rather than extending it, is a slightly better pattern, but neither are particularly lovely.
• Given StatusValue is inside libs/ it really shouldn't be dependent on code outside the libs/ folder, and yet alongside Wikimedia\Message\MessageValue and (implicitly Wikimedia\Message\…) MessageSpecifier it uses MediaWiki\Message\Converter for normalizeMessage(), ever since 951966c63c35 (in general, our totally-broken let's-put-it-in-libs-and-hope-people-keep-it-isolated approach doesn't work; this is far from the only problem area).

Happy to defer to wiser heads.

I have an easier time talking about code. Considering that the patch https://gerrit.wikimedia.org/r/1005462 triggered all this, I wonder what I'm missing in the following proposal?

class EditEntityStatus extends StatusValue {
	public static function wrap( StatusValue $status ) {
		if ( !( $status instanceof static ) ) {
			$status = static::newGood( $status->value );
		}
		if ( !( $status->getValue()['revision'] instanceof EntityRevision ) ) {
			throw new UnexpectedValueException( 'Unexpected status' );
		}
		// Can add more assertions here…
		return $status;
	}

	public function getRevision(): EntityRevision {
		// Can add more assertions here…
		return $this->getValue()['revision'];
	}

	public function getErrorFlags(): int {
		return $this->getValue()['errorFlags'] ?? 0;
	}
}

• You can create your own EditEntityStatus objects anywhere you like (I skipped some of the static methods in my example above, feel free to add as many as you like) and still pass it around as a compatible StatusValue object. As long as you don't add class-level properties but use the existing value for that, as you already pointed out.
• You can cast StatusValue back to EditEntityStatus any time you want. This can obviously only be done when we know what we get is a compatible object. But I think this restriction is unavoidable.
• You can do this safely, knowing you would get meaningful assertion failures in case some of the assumptions are broken.

What still confuses me is that the questions in this ticket are mostly unrelated to the problem at hand, when approached like this.

I have an easier time talking about code. Considering that the patch https://gerrit.wikimedia.org/r/1005462 triggered all this, I wonder what I'm missing in the following proposal?

That StatusValue is not @stable to extend. I really wish I just hadn’t noticed this, because it’s not like anyone would’ve complained if I’d just subclassed it regardless of the stable interface policy, just like several other extensions already do (and, in the case of SecurePoll, have done for many years, since long before the stable interface policy existed). I thought I could maybe just declare it stable, but from @Tgr’s comment it sounds like this is blocked on a whole raft of improvements to the class, hence my last question “who’s actually going to do this?”

In T358492#9577099, @Lucas_Werkmeister_WMDE wrote:

That StatusValue is not @stable to extend. I really wish I just hadn’t noticed this, because it’s not like anyone would’ve complained if I’d just subclassed it regardless of the stable interface policy, just like several other extensions already do (and, in the case of SecurePoll, have done for many years, since long before the stable interface policy existed). I thought I could maybe just declare it stable, but from @Tgr’s comment it sounds like this is blocked on a whole raft of improvements to the class, hence my last question “who’s actually going to do this?”

Which comment by @Tgr? I may be missing something, but think subclassing would be fine. The class is already newable, so the constructor (or lack thereof) needs to be stable.

If we make StatusValue stable to extend, this means that any protected members become stable to call/access automatically. That's probably not great for the $errors array. We can't make it private, because Status needs access to it, but it could be marked @internal. Same for getStatusArray: its contract is a bit awkward, so it should probably be @internal or @unstable.

In T358492#9577209, @daniel wrote:

Which comment by @Tgr?

This one (I hope the link works, Gerrit isn’t great at linking to comments. The one from 11:24 UTC).

In T358492#9577272, @Lucas_Werkmeister_WMDE wrote:

In T358492#9577209, @daniel wrote:

Which comment by @Tgr?

This one (I hope the link works, Gerrit isn’t great at linking to comments. The one from 11:24 UTC).

Ah, thanks.

I agree with most of what Gergö is sasying there, tough I think warnings are actually useful in some sometuations.

But I think the key bit is the "if" in this sentence: "If making it extensible will make it harder to update it, that creates more cost than benefit IMO."

If we are careful, then subclassing doesn't add to the mess. It just adds some type safety and concenience on top. It's no worse than using StatusValue directly.

I think having fatals and warnings is useful and reasonable. What seems confusing now is that we have kind-of-three levels of errors, one of which is confusingly called an "error" type error:

• StatusValue::warning() sets a "warning" error.
• StatusValue::error() sets an "error" error. This seems the most dubious. Is it supposed to mean that something definitely failed, but didn't ruin the whole operation? I can't see where this would come up besides some kind of very unusual sub-status merging, which should probably be done with custom merge logic anyway (possibly changing sub-status fatals to warnings).
• StatusValue::error() sets a "error" error (not a "fatal" error), and makes isOK() return false. Since the type is just "error", one can no longer find "which errors" caused the fatal (if any, since something could just call setOK( false ) for some reason).

It would also be nice to remove successCount/failCount. In the case of FileBackend, these fields mostly just get in the way. The success array field would remain. This field is supposed to be for "top level" results for each operation in a batch operation status. The successCount/failCount fields are for something more ill-defined, given how merge() currently adds them up.

A basic StatusValue::merge() method would simpler if:

• There where just errors/fatals and no separate "ok" value.
• We removed successCount/failCount. These sound like they should align with the success array, but they don't given how merge() currently works.
• The rarely used $overwriteValue parameter to merge() was removed.
• Better yet, if it was also named mergeErrors(), it would be more obvious what it does (and thus does not do). For example, it does not update the success array (since the success array for a sub-status would be "lower level" operation results rather than the "top level" ones for the batch as a whole. We also now have statusData, which would be clearly excluded from a method named mergeErrors().

More complex status merging could always be done by callers if they need to do something special. Right now, the class is trying to support to many fancy under-defined concepts.

StatusValue supports both MessageSpecifier and also MessageValue. The former is just a /libs technicality to pass a non-libs Message subclass of that interface. The later is a /libs class which StatusValue deals with by turning it into a Message via StatusValue::normalizeMessage(). It would be nice to just settle on MessageValue.

In T358492#9579093, @aaron wrote:

I think having fatals and warnings is useful and reasonable. What seems confusing now is that we have kind-of-three levels of errors, one of which is confusingly called an "error" type error:

See also T309859: Remove distinction between Status::error and Status::fatal.

Errors vs. warnings and fatals vs. errors are two separate implementations of the concept (differentiating between problems which resulted in the operation failing and problems which didn't). I don't think there's much sense in having both; it results in non-fatal errors sometimes being used to indicate failure and sometimes not. The way they are implemented is also full of unintuitive elements (e.g. adding a warning will cause isGood() to be false; there is a setOK() method but if you use it, it will almost certainly cause an exception later on). The lack of clarity also means warnings often don't get checked and are lost.

Wrt. messages, StatusValue accepts MessageValue as input but always returns Message via getErrors() etc. Which is of course its own can of worms, with Message being a god object that breaks unit tests, and all the weirdness with RawMessage. That's not a problem with StatusValue per se, but it's hard to isolate StatusValue from the problems of Message.

It would also be nice to remove successCount/failCount. In the case of FileBackend, these fields mostly just get in the way. The success array field would remain. This field is supposed to be for "top level" results for each operation in a batch operation status. The successCount/failCount fields are for something more ill-defined, given how merge() currently adds them up.

More complex status merging could always be done by callers if they need to do something special. Right now, the class is trying to support to many fancy under-defined concepts.

I’d probably change this “by callers” to “by subclasses” – if, say, RevDelList has a legitimate use case for successCount and failCount, let it have a custom RevDelStatus subclass with those fields and any associated merge implementations. (Possibly even two subclasses, depending on whether perItemStatus is true or false. But I haven’t looked closely at this code.) But in general, I don’t disagree that some features could be moved out of the status classes.

The “error” list (in the broader sense) is, to me, one of the core features of Status – it means that, in principle, any code dealing in terms of Status can produce or show any kind of error, with a nice localized message, without the “showing” end of the code having to know anything about which errors the “producing” code might result in. (This is where “checked” exceptions really break down. I think we have plenty of classes in Wikibase that document a set of thrown exceptions that’s only loosely related to what they can actually end up throwing in practice.) The distinction between “error” and “warning”, on the other hand, has never been relevant in my experience (for the longest time, I actually thought “warning” just meant “Status that has errors but is still OK”), and the distinction between “OK” and “good” makes some sense to me but often doesn’t seem to be handled consistently by the code.

Change 1005771 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] Make StatusValue stable to extend

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

Change 1006049 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] Move Status::wrap() to StatusValue

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

FYI, I’ve now changed the Wikibase change to no longer depend on the MediaWiki changes I proposed, to unblock work on several other tasks that I already based on this refactoring. Depending on how this discussion goes, I’ll either revert it later or change it to StatusValue later, but for now Wikibase isn’t blocked on this discussion anymore – it’s just becoming one more extension that extends Status without it being stable.

Change 1005771 merged by jenkins-bot:

[mediawiki/core@master] Make StatusValue stable to extend

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

(See also T358779 for Message vs. MessageValue.)