Page MenuHomePhabricator

Determine possible packages to extract
Closed, ResolvedPublic

Description

A/C: Identify a package or packages that we could do this with.

Event Timeline

Tarrow created this task.Jul 8 2020, 1:16 PM
Ladsgroup added a comment.EditedJul 8 2020, 1:43 PM

Graph of dependencies in lib

The png version is 50MB.

That graph seems to include vendor/ (e. g. DecimalMath near the bottom, which is in data-values/number) – is it possible to get a version of the graph with only code in lib/includes/?

That graph seems to include vendor/ (e. g. DecimalMath near the bottom, which is in data-values/number) – is it possible to get a version of the graph with only code in lib/includes/?

It doesn't include vendor but it includes dependencies on outside of the code base. I can drop all of them as another picture to have a mapping of internal dependencies.

Only and only lib:

Lib but also including dependencies on other classes that start with Wikibase

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?

Restricted Application added a project: Wikidata. · View Herald TranscriptJul 16 2020, 5:34 PM

Sounds good to me, though I don’t yet see how we would fix the uses of RevisionRecord and User in EntityChange.

Sounds good to me, though I don’t yet see how we would fix the uses of RevisionRecord and User in EntityChange.

yeah for that part, I think we can move these methods of leave the class in lib. Does that make sense? It seems it's the wiring point with mediawiki.

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)

Tarrow closed this task as Resolved.Jul 20 2020, 10:22 AM
Tarrow claimed this task.

Seems like a good candidate to me then. Maybe we keep the cache in mind as either a second example or if we hit a snag with this.

Tarrow reassigned this task from Tarrow to Ladsgroup.Jul 20 2020, 10:28 AM
Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptJul 20 2020, 10:28 AM