Page MenuHomePhabricator

Move efSchemaValidate out of global scope
Closed, ResolvedPublic

Description

efSchemaValidate is a global function, and a blocker to extension registration in T87912. It's used outside EventLogging in UploadWizard, so needs a little more handling to make sure both patches are merged at the same time when it's moved to belonging to a class...

Event Timeline

Reedy created this task.Jul 20 2016, 3:31 PM
Restricted Application added a project: Multimedia. · View Herald TranscriptJul 20 2016, 3:31 PM
Restricted Application added subscribers: Zppix, Steinsplitter. · View Herald Transcript
Restricted Application added a subscriber: Matanya. · View Herald TranscriptJul 20 2016, 3:40 PM

Change 300046 had a related patch set uploaded (by Reedy):
Move efSchemaValidate to EventLogging class

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

Change 300047 had a related patch set uploaded (by Reedy):
Update call to efSchemaValidate

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

Jdforrester-WMF moved this task from Untriaged to Doing on the Multimedia board.Jul 20 2016, 9:02 PM

Hold on… is efSchemaValidate() a generic function to validate some JSON data against some JSON schema? Why is it in EventLogging, and why, for all that is holy, do we use it in UploadWizard?

I'm pretty sure this is the same kind of JSON schema as the one used in MediaWiki core to validate extension.json files. That would imply MediaWiki core has a method to do this. Why do we need efSchemaValidate() at all?

Hold on… is efSchemaValidate() a generic function to validate some JSON data against some JSON schema? Why is it in EventLogging, and why, for all that is holy, do we use it in UploadWizard?
I'm pretty sure this is the same kind of JSON schema as the one used in MediaWiki core to validate extension.json files. That would imply MediaWiki core has a method to do this. Why do we need efSchemaValidate() at all?

From IRC:

[16:38:00] <marktraceur> Hmm Reedy does efSchemaValidate have a reasonable alternative in core yet? Last I checked it was lackluster.
[16:38:12] <Reedy> Not sure, I'd presume not
[16:38:26] <Reedy> I was gonna just move it to the "EventLogging" class for now
[16:38:32] <Reedy> UW seems to be the only other consumer
[16:39:50] <Reedy> marktraceur: Unless someone says just move it to core
[16:39:56] <marktraceur> Reedy: UW and now FileAnnotations :)
[16:40:06] <Reedy> orly?
[16:40:17] <marktraceur> I used the code from UW as a template, and when legoktm suggested the alternative, I found it to be lacking entirely
[16:40:38] <marktraceur> Reedy: We use efSchemaValidate to lay out the schema for the File Annotations namespace

Well… @MarkTraceur, when did you last check? ;)

<marktraceur> MatmaRex: Sorry, I'm not at my computer right now, but I last checked a week or two ago when I was writing the schema for FileAnnotations.
<marktraceur> MatmaRex: And legoktm gave me a convoluted option that didn't seem like it would do the same thing anyway.

The library we use for extension.json validation doesn't support localized error messages. It's also only a dev-dependency right now, but it has passed security review and could become a required dependency.

Change 300755 had a related patch set uploaded (by Reedy):
Promote justinrainbow/json-schema from dev to always required

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

Change 300046 merged by jenkins-bot:
Move efSchemaValidate to EventLogging class

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

Change 300047 merged by jenkins-bot:
Update call to efSchemaValidate

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

Reedy closed this task as Resolved.Jul 26 2016, 6:40 PM
Reedy claimed this task.

Change 300755 abandoned by Krinkle:
Promote justinrainbow/json-schema from dev to always required

Reason:
Closing in favour of potentially having UploadWizard use this directly in its own composer.json file (which we'd aggregate in mediawiki-vendor for wmf).

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