A/C: Identify a package or packages that we could do this with.
|Open||None||T256058 Extract PHP package/library for "changes" as a "subtree" from the Wikibase monorepo|
|Resolved||Ladsgroup||T257439 Determine possible packages to extract|
Looking at the graph, a meaningful case to take out is Wikibase\Lib\Changes. There are some uses of mediawiki here and there but they are mostly easily fixable (like MWException -> Exception) and unlike the SimpleCache bit, it's a fairly large code base (10 classes and 2 interfaces). What do you think?
I don’t think leaving EntityChange (and its subclass ItemChange) behind in Lib would be great – I feel like those should be the central classes of the “changes” package. But this reminds me again of what I wrote in T255110#6224767:
Instead, I think we should challenge the role of this EntityChange class overall. It’s used in Client, so why can it even expect to access a Repo class without crashing on wikis where Repo doesn’t exist? And the answer is that while the class as a whole is used on Client and Repo, this particular method, setRevisionInfo() is only called from Repo. I think this reflects a more general split within this EntityChange class: it has “getter” methods which can be called from Client and Repo, and “setter” methods such as setRevisionInfo() which can only be called from Repo. (With one exception: Client’s InjectRCRecordsJob::getChange() calls $change->setField(), to update the “info” field.) So maybe we should split this class into two parts, so that the parts that can only be called by the Repo, including setRevisionInfo(), are also defined in the Repo extension, where they are free to access classes like EntityContent.
I think both RevisionRecord and User are only used in the “setter” classes, so maybe we can extract the “setter” part and leave that in Lib, but move the rest of EntityChange and ClientChange to the separate package?
Yes, my comment was a very local/band-aid type of solution. Mostly to avoid changing too much (which has risk of breaking things) but I totally agree with you. This class at its current shape is violating ISP and splitting it makes lots of sense (not to mention we should avoid setters in general to make classes immutable which would make them more predictable)