This doesn't appear to be a problem in the 2017 wikitext editor, only the old wikitext editor. Unfortunately on some wikis these aren't enabled on Template pages (i.e. where the bug was first detected, Turkish Wikipedia: https://phabricator.wikimedia.org/T257809#6332885 )

I previously thought this was a TemplateData extension issue, but given that this is handled correctly by the 2017 wikitext editor, it might be a core problem? Not sure I tagged the right projects.

https://doc.wikimedia.org/mediawiki-core/master/php/classStatusValue.html#a173df704be03a150626c89412f14fe04 looks like newFatal is a StatusValue fcn now? Maybe things just need to be updated?

We will see if the Core Platform team can investigate this

I hit the same problem just now. Any updates on a possible solution?

I did a git bisect and found that this patch is responsible for the broken behavior: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/604839, done by @Pchelolo via T255177. I'm not sure I fully understand the patch. It looks more like a hack that was done to silence a failing test (?) in an extension (?), namely MassMessage.

I strongly suggest to revert that patch and find another solution for the original problem. Apparently it was not a mistake to "return internal edit status from EditPage", which is what the patch removed. The error message "could not be converted to int" seem to point to a line of code that tries to read from $status->value and compare that value with an integer. Based on what I found in my git bisect as well as whats written in the original task I can see a few places that contain a switch ( $status->value ), one in EditPage and two more in ApiEditPage. I suggest to add something like if ( is_int( $status->value ) ) to these places.

As far as I understand this issue is related to a recent PHP update. More recent PHP versions started to complain when unexpected things are compared. This did not happened before, which is why we have never seen this. The code did not change. PHP did. In old PHP versions a switch (which does loose == comparisons internally) would just not do anything (as it should) when the value doesn't match, no matter if one is an object and the other an int. This is what changed.

Change 656444 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] Safe-guard switch possibly doing bad comparisons

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

Change 656423 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] Revert "Do not return internal edit status from EditPage"

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

I did a git bisect and found that this patch is responsible for the broken behavior: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/604839, done by @Pchelolo via T255177. I'm not sure I fully understand the patch. It looks more like a hack that was done to silence a failing test (?) in an extension (?), namely MassMessage.

This was not a hack, this was making EditPage respect it's own contract (heh, EditPage contract...). As described in the original commit message, EditPage::internalAttemptSave returns a Status, where value has to be one of the EditPage::AS_* constants. Passing out the Status received from WikiPage::doEditContent is plain wrong - it's violating EditPage contract.

After my patch, EditPage::internalAttemptSave returns AS_END, documented as /** Status: WikiPage::doEdit() was unsuccessful */ - general saving error, no details.

EditPage::handleStatus treats AS_END like nothing happened for whatever reason. Instead, it should show an error for AS_END

[…] making EditPage respect it's own contract […] where value has to be one of the EditPage::AS_* constants.

I see. Thanks for the explanation. Still I wonder: What the patch did was not only dropping values that aren't one of these constants, or dropping values that aren't integers. It was dropping all values, even valid ones.

In other words: An alternative solution might be to drop non-integer values much earlier. I'm not absolutely sure where the best place for this line of code is. Probably in the same place where the original patch enforced the AS_END.

if ( !is_int( $doEditStatus->value ) ) {
    $doEditStatus->value = self::AS_PARSE_ERROR;
}

The small patch I uploaded does this is_int check as late as possible, in the failing switch.

As far as I understand the code I have seen so far, AS_END is reserved for errors that happen in core and are already communicated to the user in another way. The line $this->hookError in the default: section of that switch tells me that extensions (or whatever is triggered via a hook) are allowed to set the value to anything else (or leave the default null untouched). The assumption seems to be that extensions can return errors, but not necessarily with one of the pre-defined integer values.

Forcing the value to be AS_END breaks this assumption.

Using AS_PARSE_ERROR instead is another option. This will display the error message as well.

Even the wikitext 2017 editor experience is unhelpful. I learn "Syntax error in JSON." with no line number, and no error highlighting in the source view.

What I would prefer here is that we *allow* the user to save bad TemplateData JSON, and instead of showing an error on save, we show it as a warning when viewing the page. This is consistent with other extensions such as Cite, which *never* blocks save but instead injects bold red text in the content. (For comparison, we discuss how to better render reference errors in T238061.)

The JSON errors don't display in the wikitext 2010 editor either.

I'm merging the revert. It looks correct to me, and @Pchelolo said on the patch that it's acceptable to revert. I'll close this task, and leave T255177 open for any follow-up work.

@awight You're right, there's a separate task: T52511 (which was mistakenly closed, I just reopened it).

Change 656423 merged by jenkins-bot:
[mediawiki/core@master] Revert "Do not return internal edit status from EditPage"

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

Change 656444 merged by jenkins-bot:
[mediawiki/core@master] Safe-guard switch possibly doing bad comparisons

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

In T258790#6819135, @awight wrote:

Even the wikitext 2017 editor experience is unhelpful. I learn "Syntax error in JSON." with no line number, and no error highlighting in the source view.

What I would prefer here is that we *allow* the user to save bad TemplateData JSON, and instead of showing an error on save, we show it as a warning when viewing the page. This is consistent with other extensions such as Cite, which *never* blocks save but instead injects bold red text in the content. (For comparison, we discuss how to better render reference errors in T238061.)

I strongly object to this, at least in TemplateData's and surrounding infrastructure's current iteration.

When template data is just the documentation, that's fine. The message that it is broken is in the same place it can be fixed. The problem is this JSON is used elsewhere not directly in the page.

The problem is we are using this JSON elsewhere and it affects behaviour, and when someone saves bad JSON it causes cascading failures, and it does so completely silently. When this happens in citoid for instance - and admittedly I guess this is my fault - there's no way for the user to see what's broken and how to fix it. Most documentation pages are able to be edited by anyone too, unlike the code for the template itself which tends to be locked.

We actually have a task open to do completely the opposite, which is to prevent people from editing the JSON in wikitext at all - personally I think this goes too far. T207139

I think the real solution to this problem is probably to have the json on its own page -> T56140.

In T258790#6848365, @Mvolz wrote:

In T258790#6819135, @awight wrote:

What I would prefer here is that we *allow* the user to save bad TemplateData JSON

I strongly object to this, at least in TemplateData's and surrounding infrastructure's current iteration.

I see what you mean and the argument certainly makes sense. In general, I think "fail fast" and "fail closed" are healthy behaviors and I tend to do so whenever possible. But maybe you can share your thoughts about the consistency point here? MediaWiki is not designed to fail closed, I only know of partial attempts to fail fast when editing such as the EventLogging schema editor, where valid JSON is enforced but not the schema itself. Wikitext templates are another precedent, these will cause terrible side-effects when broken, but we don't have validation.

Not to say that the current situation is a good one, but I worry that strong validation will make wikitext much harder to edit. Let's say an editor makes good-faith changes to template documentation, but introduces some small or large error to the TemplateData block. With soft validation, the page is saved, and a warning appears. Another editor can easily fix the mistake in a later change. With hard validation, the first editor will be forced to debug (currently, without any indication of the error!) and may give up, losing their entire edit. I worry that the effect is that very few editors will be interested in making changes to TemplateData.

Can we make the fallback mechanism more polite, so that failures don't cascade? I would have imagined that the fallback would be that the TemplateData API simply switches over to attempt autodetecting of template params.

I like the idea of putting TemplateData json in its own page, or in a multi-content revision slot. Jade has a similar need, but there's no obvious mechanism for associating wiki entities, yet.