⚓ T275976 JsonContent or a replacement for it should be stable to extend

⚓ T275976 JsonContent or a replacement for it should be stable to extend
Page Menu
Phabricator
Create Task
Maniphest
T275976
JsonContent or a replacement for it should be stable to extend
Open, Low
Public
Actions
Edit Task
Edit Related Tasks...
Create Subtask
Edit Parent Tasks
Edit Subtasks
Merge Duplicates In
Close As Duplicate
Edit Related Objects...
Edit Commits
Edit Mocks
Mute Notifications
Protect as security issue
Assigned To
None
Authored By
Urbanecm_WMF
Feb 27 2021, 9:56 PM
2021-02-27 21:56:48 (UTC+0)
Tags
MediaWiki-ContentHandler
(Backlog)
User-Daniel
(Inbox)
Platform Team Workboards (MW Expedition)
(Unsorted pile)
MW-1.38-notes (1.38.0-wmf.22; 2022-02-14)
Patch-Needs-Improvement
Referenced Files
None
Subscribers
Aklapper
Daimona
daniel
Jdforrester-WMF
Legoktm
Nikerabbit
SD0001
Urbanecm_WMF
Description
I started looking into custom extension content models, and I found that
JsonContent
core class is not stable to extend. Is that intended? Am I supposed to extend
TextContent
and re-implement the pure JSON part? Or is this just an oversight?
Details
Related Changes in Gerrit:
Subject
Repo
Branch
Lines +/-
DEMO: JsonDataContentHandler
mediawiki/core
master
+203
-8
Extract JsonStructureRenderer from JsonContent
mediawiki/core
master
+261
-140
content: Document use cases for JsonContent
mediawiki/core
master
+14
-4
content: Mark JsonContent as stable to extend
mediawiki/core
master
+4
-0
Customize query in gerrit
Related Objects
Mentions
Mentioned In
T322657: Investigation: Can we display event registration actions in event page history?
T317786: Ensure message bundles are formatted nicely when viewing and editing
T307978: [JsonConfig extension support question] Using JsonConfig extension, is it possible to create "*.json" pages using the native 'json' content model type?
T273538: Make ZPersistentObject extend Content directly, not TextContent or JsonContent
T271406: Translatable modules: Content handler for MessageBundle
T275578: File bug report with core to request JsonContent @newable
Event Timeline
Urbanecm_WMF
created this task.
Feb 27 2021, 9:56 PM
2021-02-27 21:56:48 (UTC+0)
Restricted Application
added a subscriber:
Aklapper
View Herald Transcript
Feb 27 2021, 9:56 PM
2021-02-27 21:56:48 (UTC+0)
gerritbot
added a comment.
Feb 27 2021, 9:58 PM
2021-02-27 21:58:56 (UTC+0)
Comment Actions
Change 667363 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Mark JsonContent as safe to extend
gerritbot
added a project:
Patch-For-Review
Feb 27 2021, 9:58 PM
2021-02-27 21:58:56 (UTC+0)
Urbanecm_WMF
added a project:
User-Urbanecm_WMF (Engineering)
Feb 27 2021, 9:59 PM
2021-02-27 21:59:49 (UTC+0)
Nikerabbit
subscribed.
Feb 27 2021, 10:01 PM
2021-02-27 22:01:13 (UTC+0)
DannyS712
mentioned this in
T275578: File bug report with core to request JsonContent @newable
Feb 27 2021, 10:23 PM
2021-02-27 22:23:28 (UTC+0)
Urbanecm_WMF
moved this task from
Backlog
to
In review
on the
User-Urbanecm_WMF (Engineering)
board.
Feb 28 2021, 5:43 PM
2021-02-28 17:43:57 (UTC+0)
Jdforrester-WMF
subscribed.
Mar 6 2021, 7:24 PM
2021-03-06 19:24:59 (UTC+0)
Comment Actions
The advice I got from Platform was to not use JsonContent ever, as it had a lot of old-style architectural decisions that they didn't want continue. We're in the middle of switching WikiLambda to its own direct implementation of AbstractContent.
Nikerabbit
added a project:
Platform Engineering
Mar 15 2021, 12:46 PM
2021-03-15 12:46:56 (UTC+0)
Comment Actions
I am in the same situation with
. If
JsonContent
(+Handler?) is to be avoided, I would like to know more details what I should avoid in my code in addition to not extending
JsonContent
Nikerabbit
mentioned this in
T271406: Translatable modules: Content handler for MessageBundle
Mar 15 2021, 12:48 PM
2021-03-15 12:48:48 (UTC+0)
AMooney
moved this task from
Inbox
to
Tech Planning Review
on the
Platform Engineering
board.
Mar 16 2021, 5:13 PM
2021-03-16 17:13:02 (UTC+0)
daniel
subscribed.
Mar 18 2021, 9:45 AM
2021-03-18 09:45:20 (UTC+0)
Comment Actions
Copying my comment from the patch, for reference:
JsonContent is somewhat ill conceived / misleading. I think it should have a big fat warning at the top telling people that they probably do NOT want to extend it. The thing is:
JsonContent is a subclass of TextContent, which means "user editable text". So it should be used ONLY if the idea is that users will manually edit JSON, directly in a text box. Which is generally a terrible idea.
If that is not the intent, then JsonContent should not be used. It should not be misunderstood as a base class for things that get serialized as JSON.
Personally, I think we should deprecate JsonContent, rather than encourage extensions to build on it.
gerritbot
added a comment.
Mar 19 2021, 2:25 PM
2021-03-19 14:25:26 (UTC+0)
Comment Actions
Change 673500 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Extract JsonStructureRenderer from JsonContent
daniel
added a comment.
Mar 19 2021, 2:29 PM
2021-03-19 14:29:53 (UTC+0)
Comment Actions
I made a patch that marks JsonContent and JsonContentHandler as stable to extend. They are useful for things like config files (though I'd like to see YAML for that).
I pulled out the JSON structure rendering, and add a bug notice explaining when JsonContent should and should not be used. After pulling out the rendering, JsonContent is barely a hundred lines, so building on top of it will hopefully be less tempting.
Legoktm
subscribed.
Mar 20 2021, 12:01 AM
2021-03-20 00:01:07 (UTC+0)
Comment Actions
In
T275976#6924246
@daniel
wrote:
JsonContent is somewhat ill conceived / misleading. I think it should have a big fat warning at the top telling people that they probably do NOT want to extend it. The thing is:
JsonContent is a subclass of TextContent, which means "user editable text". So it should be used ONLY if the idea is that users will manually edit JSON, directly in a text box. Which is generally a terrible idea.
Well, no. It's possible to add a nice editing interface in front (MassMessage does this rather well IMO) but still let advanced users who want to edit it as raw JSON. Yes, exposing users to only JSON in a text box is a terrible idea, but that has nothing to do with the content handler representation.
I put JsonContent in core because there were a ton of extensions copying it around, including: EventLogging, UploadWizard, BookManagerv2, JsonConfig, MassMessage, none of which I would describe as "ill conceived / misleading".
If that is not the intent, then JsonContent should not be used. It should not be misunderstood as a base class for things that get serialized as JSON.
Personally, I think we should deprecate JsonContent, rather than encourage extensions to build on it.
JsonConfig provides a way to store JSON in wiki pages, with validation, PST, and a default structured representation. It also enables CodeEditor's JSON editor. It does nothing more and never promised to do so.
daniel
added a comment.
Edited
Mar 21 2021, 4:01 PM
2021-03-21 16:01:47 (UTC+0)
Comment Actions
In
T275976#6930814
@Legoktm
wrote:
Well, no. It's possible to add a nice editing interface in front (MassMessage does this rather well IMO) but still let advanced users who want to edit it as raw JSON. Yes, exposing users to only JSON in a text box is a terrible idea, but that has nothing to do with the content handler representation.
Ok, I'll take back "ill conceived" - when the intent is to maintain complex configuration or similar structures that benefit from the flexibility of JSON, and you want to expose that complexity to users, JsonContent is useful. And of course, it's easier to just let users edit JSON than to write a dedicated API and UI for editing.
My point is: JsonContent should
only
be used if the intent is to allow users to edit JSON directly (even if another way to edit). JsonContent is a TextContent, and TextContent is, by definition, user editable text. Anything that is not user editable text should not be TextContent. There are several places in MediaWiki that do typechecks for TextContent to see if the content should be treated as text, or as serialized data.
I think JsonContent is misleading, because people who want to maintain "structured data" on a page may think building on top of JsonContent is the obvious and correct thing to do, if they want to serialize to JSON. This leads to code that is based on untyped nested arrays rather than a proper domain model, and additional work do prevent direct editing and replace the default rendering.
Would you agree that for structured data that should not be editable directly and needs custom rendering, JsonContent should not be used? If yes, how would you document that on the class? If no, how do you think such a user cases benefits from JsonContent?
JsonConfig provides a way to store JSON in wiki pages, with validation, PST, and a default structured representation. It also enables CodeEditor's JSON editor. It does nothing more and never promised to do so.
Yes - it's the right thing to use if you want users to
edit
JSON! But I think it's the wrong thing to use if you only want to serialize to JSON, but do not want users to edit JSON. That's what I want to make clear in the class documentation.
Legoktm
added a comment.
Mar 22 2021, 7:17 AM
2021-03-22 07:17:10 (UTC+0)
Comment Actions
In
T275976#6932577
@daniel
wrote:
My point is: JsonContent should
only
be used if the intent is to allow users to edit JSON directly (even if another way to edit).
Yes, but I'm not sure why you're against allowing this as a possibility. I think in the vast majority of cases there's nothing wrong with allowing such arbitrary editing.
JsonContent is a TextContent, and TextContent is, by definition, user editable text. Anything that is not user editable text should not be TextContent. There are several places in MediaWiki that do typechecks for TextContent to see if the content should be treated as text, or as serialized data.
Surely direct editing should be controlled by... ContentHandler::supportsDirectEditing() and not instanceof TextContent? Wasn't this the whole point of introducing that function?
In any case, in my experience extending TextContent means that a lot of things just work and you just override what you need. Extending AbstractContent is painful and you have to reimplement a bunch of stuff (it's been years since I tried fwiw).
I think JsonContent is misleading, because people who want to maintain "structured data" on a page may think building on top of JsonContent is the obvious and correct thing to do, if they want to serialize to JSON. This leads to code that is based on untyped nested arrays rather than a proper domain model, and additional work do prevent direct editing and replace the default rendering.
Would you agree that for structured data that should not be editable directly and needs custom rendering, JsonContent should not be used? If yes, how would you document that on the class? If no, how do you think such a user cases benefits from JsonContent?
What exactly is your proposed alternative? Are all the extensions currently utilizing JsonContent doing something wrong that needs calling out in the documentation? JsonContent is not the answer if you're trying to implement Wikibase, but for just about every other structured wiki page thing, it seems to work pretty well.
daniel
added a comment.
Edited
Mar 22 2021, 6:09 PM
2021-03-22 18:09:52 (UTC+0)
Comment Actions
In
T275976#6933328
@Legoktm
wrote:
Yes, but I'm not sure why you're against allowing this as a possibility. I think in the vast majority of cases there's nothing wrong with allowing such arbitrary editing.
If what you are storing is basically configuration, then free-form editing (plus some validation) is fine. If it's actually data, then not so much.
There are basically two kinds of cases: "store JSON on a wiki page" and "store data on a wiki page". For the latter, the data can be encoded as JSON, sure, but it should not be using JsonContent.
Surely direct editing should be controlled by... ContentHandler::supportsDirectEditing() and not instanceof TextContent? Wasn't this the whole point of introducing that function?
Yes, you can control it via ContentHandler::supportsDirectEditing(). But if you don't want direct editing, why do you extend TextContent? What do you gain from it?
If you take out the code for rendering the JSON structure, all that is left in JsonContent is code for prettifying the JSON itself. Which you don't need if you don't want to edit it. JsonContentHandler is even more trivial.
In any case, in my experience extending TextContent means that a lot of things just work and you just override what you need. Extending AbstractContent is painful and you have to reimplement a bunch of stuff (it's been years since I tried fwiw).
AbstractContent has no abstract methods, you just add your desired data to it. ContentHandler has three abstract methods: serializeContent(), unserializeContent(), and makeEmptyContent(). All of them are trivial.
If you build on top of TextContent, you get these things "for free":
Diffs, line based. This can work OK if the JSON is nicely formatted, and is fairly human readable. For complex data structures, you probably want a custom diff representation, like Wikibase does. Line based diffs are going to be useless.
Template-transclusion (as text). You probably don't want that for data structures.
Line-based 3-way merged of edit conflicts. Which for a JSON data structure likely to explode, you are better off without. If you want conflict resolution, you'll have to build one that understands the semantics of the data and doesn't operate on text.
Full text search, based on the JSON data. Which will includes field names, which is probably not what you'd want.
Conversion to and from other text-based content formats. You can re-interpret your data structure as wikitext. Probably not desirable for complex data structures.
This is why I think that JsonContent makes it easy to do the wrong thing for extensions that want to store "data".
What exactly is your proposed alternative? Are all the extensions currently utilizing JsonContent doing something wrong that needs calling out in the documentation? JsonContent is not the answer if you're trying to implement Wikibase, but for just about every other structured wiki page thing, it seems to work pretty well.
It's perfectly fine for something like EntitySchema or EventLogging. It's dubious for something like JADE, UploadWizard or MediaUploader (make users edit JSON because building a UI is too much work). It's the wrong thing for something like WikiLambda.
Jdforrester-WMF
added a comment.
Mar 22 2021, 6:21 PM
2021-03-22 18:21:08 (UTC+0)
Comment Actions
Perhaps there should be a
DataContent
sub-class of
AbstractContent
which we encourage things to use, and provide a
JsonDataContent
type without the direct-edit features?
daniel
added a comment.
Edited
Mar 24 2021, 11:10 AM
2021-03-24 11:10:31 (UTC+0)
Comment Actions
In
T275976#6935801
@Jdforrester-WMF
wrote:
Perhaps there should be a
DataContent
sub-class of
AbstractContent
which we encourage things to use, and provide a
JsonDataContent
type without the direct-edit features?
We can have a JsonDataContentHandler and JsonDataContent make sense to me if they build on top of JsonUnserializable data objects. They would be trivial, but would provide guidance.
By the way - it should be possible to switch a given content model's handler from JsonContent to JsonDataContent (or vice versa) without breaking existing content in the database, as long as the serialiozation is compatible and the model name is the same.
What I don't want to encourage is nested arrays as the internal representation of structured data.
gerritbot
added a comment.
Mar 24 2021, 11:57 AM
2021-03-24 11:57:35 (UTC+0)
Comment Actions
Change 674572 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] DEMO: JsonDataContentHandler
daniel
added a project:
User-Daniel
Mar 24 2021, 9:35 PM
2021-03-24 21:35:51 (UTC+0)
daniel
triaged this task as
Low
priority.
Mar 25 2021, 9:02 AM
2021-03-25 09:02:45 (UTC+0)
daniel
edited projects, added
Platform Team Workboards (MW Expedition)
; removed
Platform Engineering
Mar 25 2021, 9:31 PM
2021-03-25 21:31:49 (UTC+0)
Urbanecm_WMF
moved this task from
In review
to
Radar
on the
User-Urbanecm_WMF (Engineering)
board.
Apr 14 2021, 5:26 PM
2021-04-14 17:26:18 (UTC+0)
Jdforrester-WMF
mentioned this in
T273538: Make ZPersistentObject extend Content directly, not TextContent or JsonContent
May 10 2021, 7:41 PM
2021-05-10 19:41:10 (UTC+0)
SD0001
subscribed.
Jan 9 2022, 12:36 PM
2022-01-09 12:36:11 (UTC+0)
Krinkle
renamed this task from
JsonContent is not safe to extend
to
JsonContent should be stable to extend
Feb 6 2022, 6:00 AM
2022-02-06 06:00:43 (UTC+0)
Krinkle
assigned this task to
Urbanecm_WMF
Krinkle
updated the task description.
(Show Details)
Jdforrester-WMF
renamed this task from
JsonContent should be stable to extend
to
JsonContent or a replacement for it should be stable to extend
Feb 7 2022, 2:27 PM
2022-02-07 14:27:14 (UTC+0)
gerritbot
added a comment.
Feb 8 2022, 11:07 AM
2022-02-08 11:07:19 (UTC+0)
Comment Actions
Change 760910 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):
[mediawiki/core@master] Clarify use case for JsonContent
gerritbot
added a comment.
Feb 8 2022, 11:12 AM
2022-02-08 11:12:34 (UTC+0)
Comment Actions
Change 667363
merged
by jenkins-bot:
[mediawiki/core@master] content: Mark JsonContent as stable to extend
ReleaseTaggerBot
added a project:
MW-1.38-notes (1.38.0-wmf.22; 2022-02-14)
Feb 8 2022, 12:00 PM
2022-02-08 12:00:19 (UTC+0)
gerritbot
added a comment.
Feb 8 2022, 10:54 PM
2022-02-08 22:54:22 (UTC+0)
Comment Actions
Change 760910
merged
by jenkins-bot:
[mediawiki/core@master] content: Document use cases for JsonContent
DAlangi_WMF
moved this task from
Unsorted pile
to
Doing
on the
Platform Team Workboards (MW Expedition)
board.
May 6 2022, 1:46 PM
2022-05-06 13:46:38 (UTC+0)
Jasonkhanlar
mentioned this in
T307978: [JsonConfig extension support question] Using JsonConfig extension, is it possible to create "*.json" pages using the native 'json' content model type?
May 10 2022, 6:42 AM
2022-05-10 06:42:53 (UTC+0)
daniel
moved this task from
Doing
to
Unsorted pile
on the
Platform Team Workboards (MW Expedition)
board.
May 24 2022, 8:20 AM
2022-05-24 08:20:41 (UTC+0)
Urbanecm_WMF
removed
Urbanecm_WMF
as the assignee of this task.
Aug 6 2022, 3:14 PM
2022-08-06 15:14:29 (UTC+0)
Comment Actions
Not actively working on this.
Urbanecm_WMF
removed a project:
User-Urbanecm_WMF (Engineering)
Aug 11 2022, 10:39 AM
2022-08-11 10:39:20 (UTC+0)
Nikerabbit
mentioned this in
T317786: Ensure message bundles are formatted nicely when viewing and editing
Oct 21 2022, 9:03 AM
2022-10-21 09:03:49 (UTC+0)
Daimona
subscribed.
Nov 10 2022, 6:58 PM
2022-11-10 18:58:57 (UTC+0)
Daimona
mentioned this in
T322657: Investigation: Can we display event registration actions in event page history?
Nov 11 2022, 5:11 PM
2022-11-11 17:11:12 (UTC+0)
Pppery
edited projects, added
Patch-Needs-Improvement
; removed
Patch-For-Review
Oct 10 2023, 5:24 AM
2023-10-10 05:24:18 (UTC+0)
gerritbot
added a comment.
Mon, Mar 30, 12:44 PM
2026-03-30 12:44:34 (UTC+0)
Comment Actions
Change #673500
abandoned
by Daniel Kinzler:
[mediawiki/core@master] Extract JsonStructureRenderer from JsonContent
gerritbot
added a comment.
Mon, Mar 30, 12:45 PM
2026-03-30 12:45:21 (UTC+0)
Comment Actions
Change #674572
abandoned
by Daniel Kinzler:
[mediawiki/core@master] DEMO: JsonDataContentHandler
Log In to Comment
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct.
Wikimedia Foundation
Code of Conduct
Disclaimer
CC-BY-SA
GPL
Credits