Page MenuHomePhabricator

Community configuration: References that do not have a dedicated control assigned should work normally
Open, HighPublic5 Estimated Story Points

Description

In T357718: Support schema references, we added support for JSON Schema references. The referenced field's name is then used to determine what control type is used. For example, the MediaWiki.Extension.CommunityConfiguration.Schemas.MediaWiki.MediaWikiDefinitions::PAGE_TITLE reference is used to trigger the PageTitleControl.

Unfortunately, when one uses references without intending to trigger a special control type, and uses an unrecognized reference name, it does not work. The form just renders itself empty, while I would expect it to render using whatever the default control type would be used, see screenshot:

image.png (382×2 px, 52 KB)

There is nothing relevant logged in the console.

Used schema

This is the schema I was using as part of T360471:

class GrowthDefinitions extends JsonSchema {

	public const TEMPLATE_BASED_TASK = [
		self::TYPE => self::TYPE_OBJECT,
		self::PROPERTIES => [
			'disabled' => [
				self::TYPE => self::TYPE_BOOLEAN,
				self::DEFAULT => false,
			],
			'templates' => [
				self::REF => [ 'class' => MediaWikiDefinitions::class, 'field' => 'PageTitles' ],
			],
		]
	];
}

class SuggestedEditsSchema extends JsonSchema {

	public const copyedit = [
		self::REF => [ 'class' => GrowthDefinitions::class, 'field' => 'TEMPLATE_BASED_TASK' ],
	];
}

When I copy TEMPLATE_BASED_TASK under SuggestedEditsSchema::copyedit, it works as expected.

Solution:

Instead of using $ref name to determine what component to display, use a class field name, which would include the name of the component (possibly represented as a PHP class name). Then, if we inline the references when processing the schema (presumably through a flag to ReflectionSchemaSource::loadAsSchema()), we can use that new field to determine the component on the client side, without having to traverse the schema again.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Urbanecm_WMF moved this task from Inbox to Up Next on the Growth-Team board.

This is happening, because the schemaTypeIs() function receives a schema that looks like this:

{
  "$ref": "#/$defs/GrowthExperiments.Config.Schemas.GrowthDefinitions::TEMPLATE_BASED_TASK",
  "default": null
}

but we do not have any information about the type of the underlying field. In this particular case, TEMPLATE_BASED_TASK has enough information for the form generating logic to be able to generate a form, but it might possibly have references to different fields which then have enough information.

The good question is...what's the way out of this? Should we...

  1. ...put the control name to JSONSchema itself (rather than (ab)using the reference name)
  2. ...introduce inlined references (and either use $refs or literally copy the content over, depending on what flags are used)
  3. ...expose the UI schema concept, and fully separate how a field is rendered from how a field looks like

Number 3 seems like the best solution here, but it's also the hardest one to implement.

Number 1 mixes rendering concerns with data specification concerns, which is not a good idea no matter what (and it breaks the modularity of CommunityConfiguration). So, I would rather not have us go that way.

Number 2 seems like a hack – more importantly, it is a hack that would live in Core (since that's where ReflectionSchemaSource lives). So, it is probably also not a good idea.

Wondering whether there is an easy-to-implement solution that also wouldn't feel like worsening things even more in the long term?

This was discussed with @daniel and @Mooeypoo this Thursday. There are two solutions @Sgs and me need to choose from:

  1. Instead of using $ref name to determine what component to display, use a class field name, which would include the name of the component (possibly represented as a PHP class name). Then, if we inline the references when processing the schema (presumably through a flag to ReflectionSchemaSource::loadAsSchema()), we can use that new field to determine the component on the client side, without having to traverse the schema again.
  2. Do not allow references that go more than one level (in this case, this would require copy-pasting the same fields ~6 times – for each suggested edit task type)

As of now, I'm leaning towards the first solution. Let's discuss this on Monday, decide and then we can implement any changes and unblock T360471.

KStoller-WMF set the point value for this task to 5.

As of now, I'm leaning towards the first solution. Let's discuss this on Monday, decide and then we can implement any changes and unblock T360471.

@Sgs IIRC, we're both leaning towards the first solution. Should we formalize that decision? Or do you need some more time to think about the implications here?

While I agree with the problem description The form just renders itself empty, while I would expect it to render using whatever the default control type would be used I'm not sure that inlining all references for an schema is the best approach. I'm curious, from the three proposals in T362098#9698720, why isn't adding support for resolving references in schemaTypeIs a valid alternative?

On my exploration to see why the editor was rendering as empty, and if resolving references in the client would be an option, I took the following steps:

  • Resolve references in schemaTypeIs, 1023109, the form rendered two labels for each object (copyedit and expand), but still no controls present because our "DispatchRenderer" does not know how to render a referenced schema.
  • extractRef started failing, found and filed T363111: Referenced definitions are not normalized, tentative fix 1023108
  • Then resolve references in ObjectControl which is our entry point for schema rendering, 1023110, the form renderes correctly (although it's missing labels, because the i18n schema traverse also needs reference support, easy to fix in a follow-up)

I think the "reference" approach for targetting UI elements should still be possible with the current architecture, and we just stumbled into an stack of issues. Schema traversal could definitely be improved on the client as we do at least two walks on the JSON schema structure (one for the text keys and one for the UI schema generation), aside from the main render code path which also traverses the structure in a way. I think we could use some third party library (eg: json-schema-tools/traverse) or build our own custom. It would also simplify the current client code.

Let me know if you think this diagnosis is accurate and the possible fixes seem fair or you still find strong arguments for the so called "first solution" of inlining references. cc @daniel @Urbanecm_WMF @Mooeypoo

Change #1023131 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/core@master] DNM: Add support for inlined references

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

While I agree with the problem description The form just renders itself empty, while I would expect it to render using whatever the default control type would be used I'm not sure that inlining all references for an schema is the best approach. I'm curious, from the three proposals in T362098#9698720, why isn't adding support for resolving references in schemaTypeIs a valid alternative?

As that'd mean having to keep schema processing/traversing logic in two places: PHP backend (in ReflectionSchemaSource and friends) and JS frontend. I do not really see why should we be doing that. If you do think doing so is a good idea, no strong objections from my end.

On my exploration to see why the editor was rendering as empty, and if resolving references in the client would be an option, I took the following steps:

Thanks for the investigation! I just uploaded a patch that I was experimenting with before I saw you claimed the task. Please feel free to use it or ignore it as you see fit – it was an attempt to implement the first solution.

Let me know if you think this diagnosis is accurate and the possible fixes seem fair or you still find strong arguments for the so called "first solution" of inlining references. cc @daniel @Urbanecm_WMF @Mooeypoo

I think what you wrote is indeed accurate. Personally, I am not wed to a particular solution, and I would be happy with either of the discussed solutions.

While I agree with the problem description The form just renders itself empty, while I would expect it to render using whatever the default control type would be used I'm not sure that inlining all references for an schema is the best approach. I'm curious, from the three proposals in T362098#9698720, why isn't adding support for resolving references in schemaTypeIs a valid alternative?

As that'd mean having to keep schema processing/traversing logic in two places: PHP backend (in ReflectionSchemaSource and friends) and JS frontend. I do not really see why should we be doing that. If you do think doing so is a good idea, no strong objections from my end.

The reasons for traversing the schema in both client and server are a mix of technology constraints and design decisions. At the moment we are performing document traversals in three spots in the CommunityConfiguration project (see list below):

  1. JsonSchemaBuilder::getRootSchema which calls ReflectionSchemaSource::loadAsSchema and then JsonSchemaTrait::normalizeJsonSchema. Necessary for serializing the schema and critical since it is hit in the "validate" path.
  2. JsonForm.vue which renders the final HTML kind of traverses the schema in a "Vue iterative" way. Intrinsic to the technology we picked, Vue.js. Even in an SSR environment it would exist so we cannot reuse another traversal for this, although we might be able to optimize it (eg: asynchronously compute data necessary for a given control).
  3. i18n.js::getControlsTextKeys where we traverse the schema to compute the necessary message labels for the form. Seems inappropriate to do on the client and it impacts the first meaningful content paint. I think it should be eventually move it to the server but the computation of the labels should happen independently of the schema loading. That is because overloading ReflectionSchemaSource::loadAsSchema with computation tasks would affect the validation path as our application is designed right now.

In general, I'm missing some low level utils for managing json schema sources both in PHP and JS, but specially in JS. There are third-party tools such as traverse or lodash (get, set) that would facilitate solving some of this problems. I think this will be a relevant topic again if we decide to implement the UI schema concept.

On my exploration to see why the editor was rendering as empty, and if resolving references in the client would be an option, I took the following steps:

Thanks for the investigation! I just uploaded a patch that I was experimenting with before I saw you claimed the task. Please feel free to use it or ignore it as you see fit – it was an attempt to implement the first solution.

Let me know if you think this diagnosis is accurate and the possible fixes seem fair or you still find strong arguments for the so called "first solution" of inlining references. cc @daniel @Urbanecm_WMF @Mooeypoo

I think what you wrote is indeed accurate. Personally, I am not wed to a particular solution, and I would be happy with either of the discussed solutions.

Since 1023131 does not solve the issue of the empty form straight forward and it also suffers from the T363111 defect I'm leaning towards solving this on the client by making schemaTypeIs() resolve references and optimize later.

I think what you wrote is indeed accurate. Personally, I am not wed to a particular solution, and I would be happy with either of the discussed solutions.

Since 1023131 does not solve the issue of the empty form straight forward and it also suffers from the T363111 defect I'm leaning towards solving this on the client by making schemaTypeIs() resolve references and optimize later.

Sounds good. Note that this approach would require somehow dealing with cases when potentially multiple references have a widget assigned.

Curious why this got moved to Blocked: What is this blocked on now? The final decision, I presume?

Change #1024700 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/CommunityConfiguration@master] [WIP] Editor: render MediaWiki controls based on schema

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

Sounds good. Note that this approach would require somehow dealing with cases when potentially multiple references have a widget assigned.

Per 1:1 conversation and arguments above I agree inlined references can simplify the schema rendering in the client. Let's move on with solution (1) from T362098#9711170.

Change #1023131 merged by jenkins-bot:

[mediawiki/core@master] JsonSchemaTrait: Add support for inlined references

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

Change #1024700 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] Editor: render MediaWiki controls based on schema

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

Change #1025680 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] Bump core version requirement

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

Change #1025680 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] Bump core version requirement

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