Page MenuHomePhabricator

Use JsonCodec serialization for TOCData
Open, Needs TriagePublic

Description

For back-compat w/ existing ParserCache contents, we're currently serializing TOCData using TOCData::toLegacy(). We should use the native JsonCodec support for the TOCData object.

Event Timeline

This is blocked because JsonCodec is core-only, and can't be used from the Parsoid library without a cyclic dependency. So this task is effectively blocked until JsonCodec is brought out of core. There's a proposal for doing this at https://github.com/cscott/json-codec but I'm having trouble getting consensus on what the library should look like.

In code I've touched, and this includes before JsonCodec was introduced, I've generally not felt the need for something like that class.

My understanding is that its main value proposition is interoperability. Is that right? It's not clear to me when a piece of code would know about Wikimedia's JsonCodec library and know about a thing encoded by it, but not about the thing itself.

I've generally opted to add public toArray and fromArray (or newFromArray) directly on to public APIs instead, without any intermediary system or quasi-standard interface without clear scope for when it would/could/should be used. Even in a library, it's not a PHP or PSR standard so it presumably remains hit-and-miss whether something implements it. Unlike PHP serialization, we don't have the added risk factor of the decode step itself creating unsafe types or classes, it's inherently going to only be safe primitives or arrays containing those (and stdClass if requested), and generally don't have a one-to-many cases where the decoder doesn't know what class it's meant to become.

What is JsonCodec meant to faccilitate in this context? It seems the responsibility of knowing about TOCData goes along with knowing about ParserOutput. I don't know if we want to move that to Parsoid at some point as well, but even if we did, that would then also be okay with knowing about TOCData as then-local class in the same library. Of it it remains in core, it's presumably okay for it to know about Parsoid's class.

Change 962698 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] TOCData/SectionMetadata implements JsonCodecable

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

Change 962698 merged by jenkins-bot:

[mediawiki/services/parsoid@master] TOCData/SectionMetadata implements JsonCodecable

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

Change 967972 had a related patch set uploaded (by Sbailey; author: Sbailey):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.19.0-a3

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

Change 967972 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.19.0-a3

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

Change #975355 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] [JsonCodec] Add support for JsonCodecable

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

Change #1038445 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] [JsonCodec] Use wikimedia/json-codec to implement JsonCodec

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

Change #975355 abandoned by C. Scott Ananian:

[mediawiki/core@master] [JsonCodec] Add support for JsonCodecable

Reason:

Squashed into Ia1017dcef462f3ac1ff5112106f7df81f5cc384f.

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