Page MenuHomePhabricator

Fix RevisionStorage::update()
Closed, ResolvedPublic

Description

This is broken (it NULLs the content if External Store is in use), and I do not believe it's used in the ordinary course of user actions. It is used once by the maintenance script that caused T95580: Flow data missing on Wikimedia production wikis.

I'm not sure we can technically remove it since it extends from ObjectStorage::update(), but we could have it throw an exception.

There is a check that's meant to prevent this, but processExternalStore is not affected. Also, calcUpdates/splitUpdates removes the content fields (because it only updates the columns that changed, and the content didn't), causing it to think it needs to be inserted into External Store.

Event Timeline

Mattflaschen-WMF raised the priority of this task from to Needs Triage.
Mattflaschen-WMF updated the task description. (Show Details)
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptApr 13 2015, 3:18 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
EBernhardson renamed this task from Remove or fix RevisionStorage::update() to Fix RevisionStorage::update().Apr 13 2015, 3:51 AM
EBernhardson set Security to None.

This will have to be fixed, the upcoming plans for a suppression bitfield will require it to exist for non-standard but possible user actions.

This will have to be fixed, the upcoming plans for a suppression bitfield will require it to exist for non-standard but possible user actions.

Is this for adding the feature of suppressing past revisions while leaving the current one visible?

Yes, to make a past revisions un-viewable without just suppressing the whole thing we need to update that revision.

Any more details on what specifically is broken? Any example that gets treated incorrectly?

It NULLs out the content if External Store is in use (I guess unless you updated the content, but we would never want to update the content of an existing revision).

I could think of one reason to update the content, if someone found an XSS in parsoid. We have a hook for keeping those from being output, but we would still want to update the historical revisions. We can certainly do that without going through the ObjectManager if its ever necessay though.

I could think of one reason to update the content, if someone found an XSS in parsoid. We have a hook for keeping those from being output, but we would still want to update the historical revisions. We can certainly do that without going through the ObjectManager if its ever necessay though.

Good point. Ideally we'll be using RESTBase by then, though.

Mattflaschen-WMF triaged this task as Unbreak Now! priority.

Change 203998 had a related patch set uploaded (by Mattflaschen):
Fix RevisionStorage->update

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

Change 203998 merged by jenkins-bot:
Fix RevisionStorage->update

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

Restricted Application added subscribers: Southparkfan, Luke081515, TerraCodes, Urbanecm. · View Herald Transcript