Unsurprisingly getting the same on clicking "Show changes"

@EAkinloose Just noting, given the task rename, that I can save successfully using a normal source edit — this does seem to only affect VE?

In T318083#8245151, @TheresNoTime wrote:

@EAkinloose Just noting, given the task rename, that I can save successfully using a normal source edit — this does seem to only affect VE?

Yeah, I assume it's just a classic edit conflict. Phab doesn't handle them as gracefully as MW.

In T318083#8245152, @Daimona wrote:

In T318083#8245151, @TheresNoTime wrote:

@EAkinloose Just noting, given the task rename, that I can save successfully using a normal source edit — this does seem to only affect VE?

Yeah, I assume it's just a classic edit conflict. Phab doesn't handle them as gracefully as MW.

(ah, my bad!)

Random guess: maybe caused by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762 ? It touches the API modules, and particularly some bits where they do error handling.

In T318083#8245163, @Daimona wrote:

Random guess: maybe caused by https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762 ? It touches the API modules, and particularly some bits where they do error handling.

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

A relevant (?) logstash entry

Error

• mwversion: 1.40.0-alpha
• reqId: Yyhh3XJvPugs7lb5y7JY8AAAAEg
• Find reqId in Logstash

Invalid API error code ""

Invalid API error code "{code}"

from /srv/mediawiki/php-master/includes/api/ApiUsageException.php(71)
#0 /srv/mediawiki/php-master/includes/api/ApiBase.php(1454): ApiUsageException::newWithMessage(MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit, string, NULL, NULL, integer)
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(89): ApiBase->dieWithError(string)
#2 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(133): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->forwardErrorsAndCacheHeaders(array)
#3 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(278): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->transformHTML(Title, string, NULL, NULL)
#4 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(252): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->getWikitextNoCache(Title, array, array)
#5 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(442): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->getWikitext(Title, array, array)
#6 /srv/mediawiki/php-master/includes/api/ApiMain.php(1900): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->execute()
#7 /srv/mediawiki/php-master/includes/api/ApiMain.php(875): ApiMain->executeAction()
#8 /srv/mediawiki/php-master/includes/api/ApiMain.php(846): ApiMain->executeActionWithErrorHandling()
#9 /srv/mediawiki/php-master/api.php(90): ApiMain->execute()
#10 /srv/mediawiki/php-master/api.php(45): wfApiMain()
#11 /srv/mediawiki/w/api.php(3): require(string)
#12 {main}

In T318083#8245175, @Daimona wrote:

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

The above Invalid API error code "" seems to somewhat confirm your findings @Daimona?

Tentatively marking as train blocker so it’s not missed.

In T318083#8245177, @TheresNoTime wrote:

A relevant (?) logstash entry
[...]
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(89): ApiBase->dieWithError(string)

So I imagine my guess

In T318083#8245175, @Daimona wrote:

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

could very well be correct, and the "falsey value" would be the empty string. CC'ing people from that gerrit change.

Change 833003 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/VisualEditor@master] Only die when error string is not empty

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

Looking at the relevant code, it seems that the only path which could potentially cause this error is the one leading to VRSParsoidClient::requestRestbase. Its documentation says:

@return array If successful, the value is the RESTbase server's response as an array with keys 'code', 'error', 'headers' and 'body'

So it does look like "error" is always set. That same method checks it using empty(). For a quick fix, I assume ApiParsoidTrait could do the same (instead of isset()). For a longer-term fix, I think requestRestbase() should either

1. Not include "error" if there's no error, or
2. Document what its value is when there's no error (i.e., empty string, false, null, etc.)

Using empty() could potentially be wrong in the edge (very edge) case where a falsey value is a valid error code (e.g., '0'). I don't think that could really happen, but knowing what to compare the value against would make the code more readable IMHO.

In T318083#8245186, @Daimona wrote:

In T318083#8245177, @TheresNoTime wrote:

A relevant (?) logstash entry
[...]
#1 /srv/mediawiki/php-master/extensions/VisualEditor/includes/ApiParsoidTrait.php(89): ApiBase->dieWithError(string)

So I imagine my guess

In T318083#8245175, @Daimona wrote:

FTR, I was talking about https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762/14/includes/ApiParsoidTrait.php#87 specifically. If, say, $response['error'] is always set in the new API but it's a falsey value when there's no error, I think it would die with the empty string as error msg, which is compatible with the symptoms.

could very well be correct, and the "falsey value" would be the empty string. CC'ing people from that gerrit change.

Yes, the error value is an empty string, see https://gerrit.wikimedia.org/g/mediawiki/core/+/cf950f5ea3c3355c9e1374450d1012aacae6022a/includes/libs/http/MultiHttpClient.php#596.

I have tested locally and the commit before https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/829762 works fine. I think the break is related to this commit.

Change 833003 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Only die when error string is not empty

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

Thank you for catching this!

Same issue wound up being a problem in the related patch to change away from ParsoidHelper in DiscussionTools. I'd agree with @Daimona that $response['error'] being an empty string is confusing and should at least be documented as a migration issue.

Change 833069 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/extensions/VisualEditor@master] ParsoidClient: error should be array or null

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

Change 833069 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] ParsoidClient: error should be array or null

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