Page MenuHomePhabricator

Wikibase Lua API have breaking change
Closed, ResolvedPublic

Description

mw.wikibase.description / mw.wikibase.label and similar functions now return also lang.
This is breaking change, given how Scribunto interprets multiple return values (https://www.mediawiki.org/wiki/Extension:Scribunto/Lua_reference_manual#Returning_text).

This should be either reverted/have different signature, or someone should go over all Lua Modules in all wikis and validate it is fixed. I fixed locally in hewiki 2 modules with such change: https://he.wikipedia.org/w/index.php?title=%D7%99%D7%97%D7%99%D7%93%D7%94:Wikibase&diff=prev&oldid=19262488

Details

Related Gerrit Patches:
mediawiki/extensions/Wikibase : masterCreate new Lua functions that return term, term language
mediawiki/extensions/Wikibase : wmf/1.28.0-wmf.19Don't use multiple return values
mediawiki/extensions/Wikibase : wmf/1.28.0-wmf.18Don't use multiple return values

Event Timeline

eranroz created this task.Sep 8 2016, 9:50 PM
Restricted Application added a project: Wikidata. · View Herald TranscriptSep 8 2016, 9:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
hoo added a subscriber: hoo.Sep 8 2016, 9:54 PM

The problem only arouses if the function's return values are directly returned to Scribunto, because Scribunto apparently just string appends all return values, which is very unexpected behavior for me, not consistent with Lua semantics.

Change 309477 had a related patch set uploaded (by Hoo man):
Don't use multiple return values

https://gerrit.wikimedia.org/r/309477

greg added a subscriber: greg.Sep 8 2016, 11:28 PM

Change 309477 merged by jenkins-bot:
Don't use multiple return values

https://gerrit.wikimedia.org/r/309477

Mentioned in SAL [2016-09-09T00:06:34Z] <hoo@tin> Synchronized php-1.28.0-wmf.18/extensions/Wikidata: Don't use multiple return values (T145138) (duration: 02m 24s)

Fixed on the deploy branch, but not on master, yet.

I suggest to either introduce a Boolean parameter for returning the language as second return value (mw.wikibase.label( 'Q123', true ) etc.) or to introduce another function (mw.wikibase.getLabelWithLanguage( 'Q123' ) etc.). Personally I prefer adding another parameter to the functions.

hoo triaged this task as High priority.Sep 9 2016, 12:11 AM
hoo updated the task description. (Show Details)
eranroz added a comment.EditedSep 9 2016, 5:38 AM

Just to mention another option - label can return label object rather than string. e.g

function Label.new( label, lang )
    definition={label=label, lang=lang}
    setmetatable( definition, { __index = Label, __tostring = function( self ) return self:toString() end   } )
    return definition
end
function Label:toString()
  return self.label
end
hoo added a comment.Sep 9 2016, 1:01 PM

Just to mention another option - label can return label object rather than string. e.g

function Label.new( label, lang )
    definition={label=label, lang=lang}
    setmetatable( definition, { __index = Label, __tostring = function( self ) return self:toString() end   } )
    return definition
end
function Label:toString()
  return self.label
end

I think that's even more breaking than what we have now, because you can't just string concatenate with it.

hoo added a comment.Sep 9 2016, 2:03 PM

If we change the metatable in the above example to the following, string concatenation will work:

setmetatable( definition, { __index = Label, __tostring = function( self ) return self:toString() end, __concat = function(t, str) return tostring(t) .. str end } )

But still for example the length operator (#label) wont correctly work.

daniel added a comment.Sep 9 2016, 2:07 PM

But still for example the length operator (#label) wont correctly work.

May be the lesser evil...

Or just make a new function. Calling code that want to use the language needs to be changed anyway.

Lucie added a subscriber: Lucie.Sep 9 2016, 3:29 PM

Are there strong arguments against just adding a second parameter to the function and default it to false? This way, it's not breaking anything and the new functionality is just introduced in a very smooth way. It occurs to me it would be the safest solution.

daniel added a comment.Sep 9 2016, 7:52 PM

@Lucie no problem except readability, and no real advantage over adding a new function. In both cases, old code functions as is, and code wanting access to the language needs to change the function call (either change the name, or give a parameter).

In general, boolean options should be avoided, since getFoo( $id, false, false, true ) doesn't tell you much. getFoo( $id, SUGAR | MILK ) is better, getFoo( $id, 'sugar', 'milk' ) would also work.

Also, a parameter that changes the structure or type of the return value is somewhat surprising.

hoo added a comment.Sep 19 2016, 5:46 PM

looks like we will need to cherry pick https://gerrit.wikimedia.org/r/#/c/309477/ when we make a new deployment branch

Yes, please.

I'm going to start working as usual again from Thursday on. I'll pick this up then ASAP.

Change 311711 had a related patch set uploaded (by Aude):
Don't use multiple return values

https://gerrit.wikimedia.org/r/311711

Change 311711 abandoned by Aude:
Don't use multiple return values

Reason:
don't think wmf19 will ever be deployed again, and certainly not with new wikibase code

https://gerrit.wikimedia.org/r/311711

Change 313380 had a related patch set uploaded (by Hoo man):
Create new Lua functions that return term, term language

https://gerrit.wikimedia.org/r/313380

hoo claimed this task.Sep 29 2016, 10:18 AM
hoo moved this task from Proposed to Review on the Wikidata-Sprint-2016-09-21 board.

Change 313380 merged by jenkins-bot:
Create new Lua functions that return term, term language

https://gerrit.wikimedia.org/r/313380

hoo closed this task as Resolved.Oct 3 2016, 6:08 PM
hoo moved this task from Review to Done on the Wikidata-Sprint-2016-09-21 board.
hoo removed a project: Patch-For-Review.

This has been fixed by restoring the behavior of the old functions and rather introducing new functions that return both the label/ description and its language.

The new functions are:

  • mw.wikibase.getLabelWithLang
  • mw.wikibase.getDescriptionWithLang
  • mw.wikibase.entity:getLabelWithLang
  • mw.wikibase.entity:getDescriptionWithLang

This change will (most likely) be deployed this week.