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/?

In T257439#6289541, @Lucas_Werkmeister_WMDE wrote:

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?

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

In T257439#6315416, @Lucas_Werkmeister_WMDE wrote:

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)