Page MenuHomePhabricator

Editing of content model other than wikitext fails
Closed, ResolvedPublic

Description

On beta.wmflabs.org it is currently not possible to edit a Lua module page. Both &action=edit and &action=submit fail, but content model wikitext works as usual.

This should not be deployed.

It is resulting in a page sectioneditnotsupported-title with sectioneditnotsupported-text content.

Needless to mention that no section= in URL.

Details

Related Gerrit Patches:
mediawiki/core : masterRevert "Pedantic strict equals."

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 13 2018, 7:24 AM
jcrespo added a subscriber: jcrespo.

Preventively adding MCR (please remove if that doesn't apply) in case there is some ongoing experiment on beta- apologies if my guess is wrong.

Update more than six hours later:

  • Persisting.
  • Same for JS and CSS pages in MediaWiki: and User: namespace.
  • Wikitext normal in all namespace.
daniel added a subscriber: daniel.

I don't think this is caused by MCR, but until we are sure, I moved it to the storage layer workboard.

Change 445658 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Revert "Pedantic strict equals."

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

Change 445658 merged by jenkins-bot:
[mediawiki/core@master] Revert "Pedantic strict equals."

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

Yes, fine, thank you.

Jdforrester-WMF closed this task as Resolved.Jul 17 2018, 5:45 AM
Jdforrester-WMF assigned this task to daniel.
awight added a subscriber: awight.Aug 9 2018, 6:48 PM

@daniel Thanks for finding the bad patch!

Not reopening, but I was asked to provide more explanation about what happened.

As part of my work on T58849, in which I'm trying to deprecate the dangerous "edittime" conflict detection parameter, I cleaned up some low-hanging fruit in EditPage.php. Specifically, in the patch https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/443964/ I replaced some loose equality == with strict equality ===.

The fact that this change caused a production outage suggests two things, that we should write unit tests to cover the use case, and that strict equality changed behavior because we're using false where some code expects an empty string. Once the unit test is written, we can find the offending code, include better documentation for hook or function parameters, then reintroduce strict equality to catch other integration mistakes in the future.