Page MenuHomePhabricator

Title::loadFromRow() should return a new instance of Title
Open, Needs TriagePublic

Description

Problem
In T210739 it was discovered that the mutating nature of Title::loadFromRow() caused an inconsistent state problem, since the Title object from master was mutated with data from a replica.

Solution
To keep the state in-sync, it would be better for Title::loadFromRow() to first clone $this in order to prevent mutating the state and return the cloned object.

Alternatively, we could replace usage of Title::loadFromRow() with Title::newFromRow() and deprecate the former.

Event Timeline

dbarratt created this task.Jan 24 2019, 1:55 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 24 2019, 1:55 AM
daniel added a subscriber: daniel.Jan 24 2019, 8:08 AM

I think that would make the problem worse. Either all of the factory methods in Title should return the cached instance, or none of them should.

This is why mutable "smart" objects are bad, especially if they are cached transparently.

Ideally, Title would be an immutable value object (or rather, be replaced by LinkTarget and perhaps something like a PageIdentity and PageRecord, see T208776 and T195069). We would cache page table rows in a PageStore service. Any operation that modifies the page table would be routed through the PageStore, so the PageStore can keep the cache consistent.

But that's the bright and shiny future. For now, let's either ditch the Title cache (note that we still have the LinkCache, which is also used by Title, and may also go out of whack), or use it consistently, and keep it up to date.

Anomie added a subscriber: Anomie.Jan 24 2019, 2:37 PM

Title->loadFromRow() is a loading method, it absolutely shouldn't return a new instance. Code that wants a new instance should use Title::newFromRow().

Whether Title->loadFromRow() should be a public method is a separate issue.

Tgr added a subscriber: Tgr.Jan 24 2019, 8:16 PM

WikiPage and Title are both sort of value objects for the page table, which predictably causes a mess. At least one of them should be killed or detached from row-level data. But we know that already.

WikiPage::loadFromRow could just clone the Title and avoid changing it, or we could expose whether a Title is loaded from master or replica data and do not update it when Title is from master but WikiPage is from replica. The first might break code that relied (possibly without being aware of it) on the WikiPage constructor updating the passed-in title; the second might break WikiPage logic that relies on data in Title matching data in WikiPage. IMO better not to poke working legacy code for no particular reason; replacing WikiPage with PageStore is a better place to spend effort.