Semanticmediawiki: MW 1.28 / Linker::link() rewrite

[mwjames]

[0] wrote:

For the most part, you'd use it in similar ways:
Linker::link( $title, $html, $attribs, $query );
is now:
$linkRenderer = MediaWikiServices::getInstance()
->getHtmlPageLinkRenderer();
$linkRenderer->makeLink( $title, $html, $attribs, $query );

[0] https://lists.wikimedia.org/pipermail/wikitech-l/2016-May/085634.html

[mwjames]

@legoktm We have at least two access points where Linker::link (off the top of my head) are used and I would highly appreciate that we avoid any situation where this gets broken.

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/includes/datavalues/SMW_DV_WikiPage.php#L359

[1] https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/includes/datavalues/SMW_DV_WikiPage.php#L364

[legoktm]

I think the best way we can do that is to make sure there are test cases in MediaWiki core (https://phabricator.wikimedia.org/diffusion/MW/browse/master/tests/phpunit/includes/LinkerTest.php) that cover the functionality that SMW is using to generate links - I can watch out and make sure I don't break anything, but tests are going to be the best way to make sure no one else unintentionally does as well.

@mwjames: Is writing additional test cases for core something you can work on?

[legoktm]

This landed in master about a week ago - was there any breakage?

[mwjames]

This landed in master about a week ago - was there any breakage?

Thanks for caring.

I run our integration tests locally (unit tests wouldn't help much in this case when run as mocked instance) plus [0] and so far it looks good (no reported errors that could be traced to this change).

[0] https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/jobs/134658789

[kghbln]

Actionable tasks to be assessed for SMW 3.1.0

[kghbln]

@mwjames @JeroenDeDauw This is a pretty old thing and it appears to me that only https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/src/Query/PrintRequest/Formatter.php#L57 is still containing the direct deprecated syntax. I may be wrong though. I assume that progress has been made silently to migrate SMW's code. Perhaps a quick and undirty code change can be done to be over an done with this bug for SMW 3.1?

[mwjames]

is still containing the direct deprecated syntax. I may be wrong though. I
assume that progress has been made silently to migrate SMW's code. Perhaps a
quick and undirty code change can be done to be over an done with this bug
for SMW 3.1?

So, here is my intake on this (and probably the reason I haven't
bothered) simply replacing the calls with LinkRenderer (as suggested
above) will most like break a bunch of integration tests which means
we break SMW at an output level.

I'm absolutely __not__ happy about pulling in a bunch of services and
have MediaWikiServices polluting the entire SMW namespace just to
have access to the Linker object.

In order to isolate the issue of polluting the SMW namespace, I would
suggest to create an object in the SMW\MediaWiki namespace that
mimics what Linker::link is doing.

PS: This is to sort of work that I won't do in my volunteer capacity,
it has no value, cost a bunch of time with no apparent added value (it
doesn't make the code cleaner on the contrary it pulls in a bunch of
services that need additional attention etc.) and is likely to break
something in way that only gets detected later making any clean-up
effort to cost twice the time of the initial refactoring.

PSS: If someone wants to do this, be my guest and go ahead BUT make
damn sure those parts that replace the calls are very well tested.

Cheers

On 8/22/19, Karsten Hoffmeyer [e-mail] wrote:

@mwjames @JeroenDeDauw This is a pretty old thing and it appears to me that
only
https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/src/Query/PrintRequest/Formatter.php#L57
is still containing the direct deprecated syntax. I may be wrong though. I
assume that progress has been made silently to migrate SMW's code. Perhaps a
quick and undirty code change can be done to be over an done with this bug
for SMW 3.1?

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/1590#issuecomment-523913546

[JeroenDeDauw]

We can of course create our own global function that gets the serve from global state and invokes it. That is easy and so is doing the search and replace.

It's pretty silly in itself. However the refactor on MW side looks like it allows for dependency injection which is good. Or am I missing something?

@mwjames do you have a problem with this new service being used in SMW? (I know you do not want to do all the dependency injection refactoring everywhere.) If so we could create our own interface and inject that.