daniel added a comment.
Thanks for moving this forward, Brion! Your code is pretty close to what I had in mind. I have repeated it below with some changes marked `// dk` In https://phabricator.wikimedia.org/T107595#2263968, @brion wrote: > In MediaWiki in general we're pretty lax about allowing unused data to accumulate in places like that, as long as its presence isn't harmful. Not the best practice but it has plenty of precedent. :) Indeed, and I don't think we should try to implement ACID semantics here. But I would like to design the interface in a way that provides a natural place for a transaction bracket to be implemented, if we ever have the need. > So an update pseudocode might look like: $plu = $something->getPageLookupService(); $pr = $plu->getPageRecord( $title ); // dk: $pr is a "dumb" record, not a store $initialRevId = $pr->getCurrentRevisionId(); $rs = $ps->getRevisionStore(); $rb = $rs->getRevisionBuilder( $initialRevId, $baseRevId ); // dk: the $baseRevId may not be the same as the $initialRevId $rb->setUser($user); // dk: let's avoid RequestContext $rb->setComment("awesome sauce"); $rb->updateSlot('main', $updatedTextContent); // dk: $updatedTextContent and $updatedScriptData are Content objects $rb->updateSlot('script', $updatedScriptData); $rs->apply( $rb ); // dk: or just save()? > where inside the RevisionStore\apply method there's something like: $dbw->start(); $bs = $this->blobStore(); $addedBlobs = []; if ($previousRevision) { $oldSlots = $previousRevision->getSlots(); } else { $oldSlots = []; } try { foreach( $this->slotUpdates as $name => $content ) { // dk: slot name => Content object // dk: The blob store knows nothing about revisions or slots or content models. // The idea is that the same service interface can be used for all kinds of blobs. // We could add optional suppor for meta-data there, but it's not needed. $dataUrl = $bs->saveBlob( $content->serialize() ); $addedBlobs[] = $dataUrl; // dk: the slot table associates $revId + $name to $dataUrl. We'll also want to // store content model, size and hash from the Content object. $slots[$name] = new Slot( $dataUrl, $content->getModel(), $content->getSize(), ... ); } // dk: saveRevisionRecord() has to do a lot of things: intert into the revision table, // insert into the slot table for each slot (using the new revision id), update the // page table. rev_len and rev_sha1 need to be calculated from the slots, rev_content_model // and page_content_model should be set to the main slot's model for b/c. // And it needs to check against $baseRevisionId to detect race conditions. this->saveRevisionRecord( $blah1, $blah2, $slots ); $dbw->commit(); } catch (Exception $e) { // If update failed, clean up any newly added backing blobs, which // may be in external databases, filesystems, or services. try { foreach( $addedBlobs as $dataUrl ) { $bs->deleteBlob( $dataUrl ); // dk: this goes wrong if the URL is content/hash based! } } catch (Exception $e2) { // if we can't get in to delete them, let them leak. they're safe. } try { // dk: this shoud roll back al lchanges to the page table, the revision table, and the slot table. $dbw->rollback(); } catch (Exception $e3) { // that probably means the db connection died. } throw $e; } The above code would replace much of what is in the Revision class now, in particular insertOn(). We can keep Revision around, but I'm not sure we can provide b/c for insertOn(). I suppose the update logic outlined at the top would would for now live in WikiPage for now. It would be called pretty much in the places where we now call Revision::insertOn(). Of course, WikiPage should also be refactored, but trying to do this at the same time as we introduce multi-content revisions is probably a bad idea. In fact, it's what got me stuck when I first tried to implement this. TASK DETAIL https://phabricator.wikimedia.org/T107595 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: daniel Cc: JJMC89, RobLa-WMF, Yurik, ArielGlenn, APerson, TomT0m, Krenair, intracer, Tgr, Tobi_WMDE_SW, Addshore, Lydia_Pintscher, cscott, PleaseStand, awight, Ricordisamoa, GWicke, MarkTraceur, waldyrious, Legoktm, Aklapper, Jdforrester-WMF, Ltrlg, brion, Spage, MZMcBride, daniel, D3r1ck01, Izno, Luke081515, Wikidata-bugs, aude, jayvdb, fbstj, Mbch331, Jay8g, bd808 _______________________________________________ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
