Semanticmediawiki: PropertyLabelFinder::findCanonicalPropertyLabelById returns bool

[JeroenDeDauw]

Users of this code are not expecting that which leads to warnings and likely bugs.

Warnings: https://gist.github.com/mszabo-wikia/bf2745970f308a24c908c1b4734acb37 (most are caused by this issue, some by https://github.com/SemanticMediaWiki/SemanticMediaWiki/pull/4307) (originally reported in https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/4294)

[JeroenDeDauw]

@mwjames I expect you have a better idea of how to fix this one than me

[JeroenDeDauw]

@mszabo-wikia I take it wikia is working on updating the extensions it uses again? I'm curious what the timeline there is.

[JeroenDeDauw]

And what PHP version you (will) use.

[mwjames]

@mwjames I expect you have a better idea of how to fix this one than me

There is nothing wrong with this method as false (aka boolean) is
allowed to be returned where it indicates that there is canonical
label for the property available which is not same as an empty label.

This function has been tested this way at least since 2.4, using PHP
7.4 as pretense (or better its failed test results) to claim
something being wrong here is not a valid argument.

On 9/21/19, Jeroen De Dauw [e-mail] wrote:

@mwjames I expect you have a better idea of how to fix this one than me

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

[JeroenDeDauw]

There is nothing wrong with the method, but there is something wrong with the combination of the method and its users.

Before the mismatch got silently ignored. To me it looks like there probably is a defect even if we ignore the spamming of the logs.

[JeroenDeDauw]

System tests in Maps are failing because of this: https://travis-ci.org/JeroenDeDauw/Maps/jobs/599421578

Relevant code is just creating pages with SMW annotations... https://github.com/JeroenDeDauw/Maps/blob/master/tests/System/SemanticMW/MapQueryTest.php#L61-L91

[JeroenDeDauw]

@mwjames your insights into this code would really be appreciated