TemplateData shouldn't store paramOrder as a separate list
Open, LowPublic8 Story Points

Description

If the params were stored in a list instead of an object we wouldn't need to maintain a separate paramOrder variable.

Problems with the current setup:

  • Anyone editing the JSON by hand has to know the param order is stored separately, and could try re-ordering the object instead. By-hand edits may be common for bulk changes.
  • JS clients do not guarantee to preserve key order when en/decoding JSON so just opening and closing the TemplateData dialog could result in a massive dirty diff with all the params being re-ordered.
  • Fixing key names would be trivial. Currently it is broken: T127969 and leaves the model in an invalid state.

In order to support a new format we would need to probably add a version attribute.

Esanders created this task.Feb 24 2016, 2:00 PM
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptFeb 24 2016, 2:00 PM
Esanders updated the task description. (Show Details)Feb 24 2016, 2:58 PM
Esanders updated the task description. (Show Details)Feb 24 2016, 3:02 PM

The reason that paramOrder was added was because parameters are kept as an object and not an array, and therefore we can't trust their order. IIRC we talked about switching it to an array, but I am not sure why that wasn't done - possibly just for convenience of not changing the TemplateData structure completely.

I think this is a good idea to do, though, for consistency of experience of the user and will reduce quite a lot of issues in the code - some you pointed out, and some exist with a sort of workaround in the code itself that we would be able to get rid of. They are working, but they're not really... proper.

I agree we should change the structure if possible. I'm just not sure how much work it is to adjust all the existing TemplateData strings that currently exist on wikis... it may take a while to truly deprecate this.

It's absolutely not practical to make a breaking change to the syntax of TD JSON; we've committed to never invalidating it.

It's absolutely not practical to make a breaking change to the syntax of TD JSON; we've committed to never invalidating it.

We can either support both modes simultaneously (paramOrder is already optional, and we can type check params), or provide a version attribute in the data if we want to be more explicit. Either way it wouldn't be a breaking change.

Restricted Application added a project: VisualEditor. · View Herald TranscriptFeb 24 2016, 10:39 PM

We could upgrade-on-edit, I suppose?

Jdforrester-WMF triaged this task as Low priority.Mar 1 2016, 8:12 PM
Jdforrester-WMF set the point value for this task to 8.
Jdforrester-WMF moved this task from To Triage to Backlog on the VisualEditor board.

Separately, looks like cswiki community doesn't want to impose param order (per @Dvorapa). That indicates that a separate paramOrder field is required and cannot be merged with the params object.

@Dvorapa Thanks for notifying me.

@Urbanecm You're welcome. Our issue is the exact opposite of this one

We could upgrade-on-edit, I suppose?

For on-edit in TemplateDataGenerator, an automatic upgrade is fine indeed. But for wikitext edits, upgrading is infeasible (PST on extension tags?) and imho unwanted due to dirty diffs.

Since VisualEditor reads it from the API (not from wikitext directly) parser normalisation will effectively auto-upgrade all existing blobs without requiring any dirty diffs.

This currently happens with interface text expansion (string -> object), and default values like deprecated=false, and sets={}.

[..] cswiki community doesn't want to impose param order

I propose we obsolete paramOrder (allowed for backward-compatibility, but deprecated and stripped in normalisation). We can add a new property that defines whether Consumers should standardise the parameter order. Based on that, VisualEditor would decide put parameters in a certain order, or to leave them in whatever order they are added. (Perhaps even allow manual drag-and-drop re-ordering).

Proposed specification:

paramsOrder

  • Value: null or string of either 'any', 'strict', or 'sort'
  • Default: null

Define the logical order of parameters.

If set to 'any', Consumers MAY allow users to display parameters in any order.

If set to 'strict', Consumers SHOULD display parameters in the order as defined in the params array.

If set to 'sort', Consumers SHOULD display parameters alphabetically. Authors SHOULD maintain params in alphabetical order if paramOrder is set to 'sort', however irregularities will not affect Consumers.

If set to null, Consumers MAY display parameters in any order. Consumers are RECOMMENDED to default to 'strict' order if params is of type Array.

If params is of type Object, Authors MUST NOT use 'strict' order.

This proposal depends on first changing params from Object to Array. Which also requires throwing a validation error for duplicate keys (so that edits are rejected with a descriptive error if two param objects have the same name).

We also need to throw a validation error if strict is used and wikitext has params in Object form.

Normalisation would:

  • Convert params from Object to Array.
  • If paramOrder is an array, re-order params based on it and set paramsOrder to 'strict'.
Dvorapa added a comment.EditedJun 29 2016, 12:50 AM

@Krinkle Your proposed solution looks acceptable for this, T138200 and T127969 too

I think it might be good to fix T138200 (which seems like a potential easy/quick fix) before undertaking this bigger change since deployment of this bigger change will have to be co-ordinated with Parsoid (which needs to add support for this new format in a backward compatible way), tested, deployed ... before this TemplateData change is deployed.

Krinkle moved this task from Backlog to Next-up on the TemplateData board.Jun 29 2016, 3:49 PM
He7d3r added a subscriber: He7d3r.Dec 28 2016, 9:55 AM