Page MenuHomePhabricator

breaking change in templatedata: asserting an alias must be a string
Closed, ResolvedPublic3 Estimated Story PointsFeature

Description

template parameters can be either strings or numbers, for order-based parameter.
(for instance, scribunto modules receive order-based parameters indexed by integers, not strings, i.e. frame.args[1], not frame.args["1"])

it is be perfectly legal, and pretty widely prevalent to have a named parameter with "order based" alias (paramname "author", alias 1)

the current templatedata looks like so:

<templatedata>
{
	"params": {
		"paramname": {
			"aliases": [
				1
			]
		}
}
</templatedata>

(true, order based params must be declared as strings in TD, but this limitation comes from JSON, and is not related to mw actual template behavior).

this worked as advertised until 1.38.0-wmf.16.

the latest version added a new assertion, requiring aliases to be strings, else it fails to consume the TD which was previously legal, with some error message.
this is a breaking change, rendering dozens of existing templates on hewiki "naked" (i.e., lacking templatedata, hence uneditable using VE).
such breaking change requires at the very least advance warning/notification, so we'll be able to prepare.

personally, i think this assertion is misguided, and will create some extra work for us, since some of the tools will have to examine each alias, and if/when it looks like "6", manually convert it to 6.

such breaking change is worthy of 1 demerit (not "1" demerit)

peace

Event Timeline

just realized that this error does not add the template to any tracking category. make it 2 demerits.

peace.

Aklapper renamed this task from braking change in templatedata (asserting an alias must be a string) to breaking change in templatedata: asserting an alias must be a string.Jan 8 2022, 5:49 PM

Change 752636 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/TemplateData@master] Allow aliases to be integers in addition to strings

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

thiemowmde changed the subtype of this task from "Bug Report" to "Feature Request".
thiemowmde moved this task from Incoming to In progress on the WMDE-TechWish-Maintenance board.

I checked both https://www.mediawiki.org/wiki/Extension:TemplateData as well as https://www.mediawiki.org/wiki/Help:TemplateData. I believe these are the primary documentation pages for the feature. They always said aliases must be strings. Are there documentation pages (possibly localized ones?) that say otherwise?

The fact that numbers have been partially accepted in some places was more an accident than anything. The interactive TemplateData editor for example can't understand numbers and even destroys them. I tried to make the responsible OOUI component behave a little more sane, see https://gerrit.wikimedia.org/r/751749. But this is not fully resolved yet.

I understand that it is convenient to allow numeric aliases. I uploaded a patch that adds this as an intentional feature, see https://gerrit.wikimedia.org/r/752636.

just to clarify: the issue is that this was allowed until last version, which means there are already "live" templates which violate the assertion.
this caused significant disruption: specifically, one of these templates was the hewiki equivalent of "Cite journal" - as a result, editors could not create automatic citations of articles using DOI, and could not even create manual such citations using VE (and thanks to mediawiki devs, many editors don't even know how to use source editor - kudos!).
we went and amended those templates on hewiki (can't be sure we found them all - see below :) ), but it's more than likely similar issue still exists on other projects.

bottom line: regardless of docs, when a change results in something that worked perfectly (such as those templates) to stop working, it's by definition a "breaking change".

more to the point:
please consider adding tracking category to the "real" errors, which prompted this change, i.e. stuff like

"aliases": [ {"a": "b"}, {"b": 12} ]

offtopic: i received a request to review this change (752636).
i did review it, but could not locate the "approve" button anywhere. gerrit is definitely not my forte...

peace.

Change 752775 had a related patch set uploaded (by Awight; author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/TemplateData@wmf/1.38.0-wmf.17] Allow aliases to be integers in addition to strings

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

Change 752636 merged by jenkins-bot:

[mediawiki/extensions/TemplateData@master] Allow aliases to be integers in addition to strings

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

Change 752775 merged by jenkins-bot:

[mediawiki/extensions/TemplateData@wmf/1.38.0-wmf.17] Allow aliases to be integers in addition to strings

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

awight subscribed.

Fix is going out with 1.38.0-wmf.17, which should be fully deployed this week.

Mentioned in SAL (#wikimedia-operations) [2022-01-12T12:50:48Z] <awight@deploy1002> Synchronized php-1.38.0-wmf.17/extensions/TemplateData: Backport: [[gerrit:752775|Allow aliases to be integers in addition to strings (T298795)]] (duration: 01m 07s)

thiemowmde set the point value for this task to 3.
thiemowmde moved this task from Backlog to Doing on the TemplateData board.

this was allowed until last version […]

There are different ways to look at this. According to the documentation it was not allowed. It was just not reported as an error. This was technically a bug, even marked in the code as such.

regardless of docs, when a change results in something that worked perfectly […] to stop working, it's by definition a "breaking change".

Like pretending documentation doesn't exist? As a developer with 20+ years of experience I have a different perspective. What we discuss here is what's called an "accidental feature". It might exist. It might even be useful. But it might as well break any time without notice. We even have a formal definition for situations like this.

This here is a bit on the edge. Our team tries hard to avoid such situations, especially for accidental features. This is in fact part of my job. I'm sorry it happened here. We picked this up as fast as we could and will turn it into an official feature.

please consider adding tracking category […]

I would like to. But there is nothing about categories in the TemplateData extension. The way it does validation really is an oddball in the universe of MediaWiki extensions. We would love to change this some day. But my team currently doesn't have resources for this, as our current focus is the WMDE-GeoInfo-FocusArea.

I did an insource search on hewiki, enwiki and dewiki, and it looks like there are no <templatedata> (any more) where aliases miss the quotation marks.

i did review it, but could not locate the "approve" button […]

That's fine. Thanks a lot for your input!

thiemowmde moved this task from Demo to Done on the WMDE-TechWish-Sprint-2022-01-05 board.