Wikimedia Bugzilla is closed!

Assigning to Ori as its a performance review.  Feel free to bounce this to the appropriate queue/person.

Reviewing revision 1441b9ab:

Capiunto.php line 30:
> # XXX: Rather use ScribuntoExternalLibraryPaths ?

ScribuntoExternalLibraryPaths is for pure-Lua libraries that are loaded using require() from a module that wants them.

ScribuntoExternalLibraries is for libraries that are loaded by default. A library that is implemented with callbacks into PHP code currently needs to be done using this hook, as the require() code path doesn't allow for setting things up.

Since this does appear to be a pure-Lua library that isn't of general utility, ScribuntoExternalLibraryPaths probably would be a good way to go.

Capiunto.hooks.php lines 54-55:
> $extraLibraries['mw.capiunto.Infobox'] = '\Capiunto\LuaLibrary';
> $extraLibraries['mw.capiunto.Infobox._render'] = '\Capiunto\LuaLibrary';

This probably isn't right, it'll instantiate and call \Capiunto\LuaLibrary::register() twice. Either the the classes should be different for each, or just one entry should be made in $extraLibraries.

Capiunto.hooks.php line 70:
> // @FIXME: Find a way to do this conditionally from Lua

Ideally you'd probably want to do addModules on the ParserOutput during the parse. If you were already using PHP callbacks that'd be easy, but providing a way for a pure-Lua library to do it without also allowing modules to load any random gadget could be tricky.

includes/lua/Infobox.lua line 28:
> error( name .. ' must be either of type string, number or nil' )

You'll most likely want to be specifying the 'level' parameter to your error() calls,[1] so that the error is reported at the location of the external caller rather than "Infobox.lua at line 28". The same goes for the other error() calls in your code.

 [1]: http://www.lua.org/manual/5.1/manual.html#pdf-error

includes/lua/Infobox.lua line 129:
> data = preprocess( data ),

I see you're preprocessing a lot of the fields being passed in. Note that when the fields are coming in from frame.args, they're already preprocessed and shouldn't need to be preprocessed again; 'data' here seems particularly likely to be in that situation.

includes/lua/Infobox.lua line 236:
> mw.capiunto = mw.capiunto or {}

The evolving style would point to using mw.ext.Capiunto.

includes/lua/InfoboxRender.lua line 28:
> :css( 'border-spacing', '3px' )

I have to wonder why this isn't in the stylesheet. Same for most of the other :css() calls in here (lines 40, 67, 95, 107, 162, 189).

Change 163063 had a related patch set uploaded by Hoo man:
Conditionally load mw.capiunto.Infobox._render

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

Change 163065 had a related patch set uploaded by Hoo man:
Conditionally add the "capiunto.infobox.main" RL module

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

Change 163066 had a related patch set uploaded by Hoo man:
Properly specify level for error calls

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

Change 163071 had a related patch set uploaded by Hoo man:
Don't inline hard coded CSS

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

Addressed the concerns raised by Brad (changes still in gerrit as of now).


(In reply to Brad Jorsch from comment #2)
> Reviewing revision 1441b9ab:
> 
> Capiunto.php line 30:
> > # XXX: Rather use ScribuntoExternalLibraryPaths ?
> 
> ScribuntoExternalLibraryPaths is for pure-Lua libraries that are loaded
> using require() from a module that wants them.
> 
> ScribuntoExternalLibraries is for libraries that are loaded by default. A
> library that is implemented with callbacks into PHP code currently needs to
> be done using this hook, as the require() code path doesn't allow for
> setting things up.
> 
> Since this does appear to be a pure-Lua library that isn't of general
> utility, ScribuntoExternalLibraryPaths probably would be a good way to go.
> 

As we now call out to PHP in order to add the RL module, this is no longer be possible for mw.capiunto.Infobox (see below). But I've made mw.capiunto.Infobox._render a library which can be dynamically loaded, when needed.

> Capiunto.hooks.php lines 54-55:
> > $extraLibraries['mw.capiunto.Infobox'] = '\Capiunto\LuaLibrary';
> > $extraLibraries['mw.capiunto.Infobox._render'] = '\Capiunto\LuaLibrary';
> 
> This probably isn't right, it'll instantiate and call
> \Capiunto\LuaLibrary::register() twice. Either the the classes should be
> different for each, or just one entry should be made in $extraLibraries.
> 

Fixed, now only mw.capiunto.Infobox is being added there.

> Capiunto.hooks.php line 70:
> > // @FIXME: Find a way to do this conditionally from Lua
> 
> Ideally you'd probably want to do addModules on the ParserOutput during the
> parse. If you were already using PHP callbacks that'd be easy, but providing
> a way for a pure-Lua library to do it without also allowing modules to load
> any random gadget could be tricky.
> 
Implemented this with a custom method just for adding our Capiunto RL module (in order to get around having to expose something like that).


> includes/lua/Infobox.lua line 28:
> > error( name .. ' must be either of type string, number or nil' )
> 
> You'll most likely want to be specifying the 'level' parameter to your
> error() calls,[1] so that the error is reported at the location of the
> external caller rather than "Infobox.lua at line 28". The same goes for the
> other error() calls in your code.
> 
>  [1]: http://www.lua.org/manual/5.1/manual.html#pdf-error
> 

Done

> includes/lua/Infobox.lua line 129:
> > data = preprocess( data ),
> 
> I see you're preprocessing a lot of the fields being passed in. Note that
> when the fields are coming in from frame.args, they're already preprocessed
> and shouldn't need to be preprocessed again; 'data' here seems particularly
> likely to be in that situation.
> 

I don't see a way around doing preprocess on everything right now... the problem is that we will get data from various sources and during some initial tests it turned out that preprocessing is something people just assume to be done. What we *could* do is add specific methods (or flags) that bypass (or enable?) preprocessing, but I'm not a huge fan of that.

> includes/lua/Infobox.lua line 236:
> > mw.capiunto = mw.capiunto or {}
> 
> The evolving style would point to using mw.ext.Capiunto.
> 
I'm not a fan of that as it seems overcomplicated. If you insist, I can change it, though.


> includes/lua/InfoboxRender.lua line 28:
> > :css( 'border-spacing', '3px' )
> 
> I have to wonder why this isn't in the stylesheet. Same for most of the
> other :css() calls in here (lines 40, 67, 95, 107, 162, 189).
Fixed.

Change 163066 merged by jenkins-bot:
Properly specify level for error calls

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

(In reply to Marius Hoch from comment #7)
> Addressed the concerns raised by Brad (changes still in gerrit as of now).
> 
> 
> (In reply to Brad Jorsch from comment #2)
> > Reviewing revision 1441b9ab:
> > 
> > Capiunto.php line 30:
> > > # XXX: Rather use ScribuntoExternalLibraryPaths ?
> > 
> > ScribuntoExternalLibraryPaths is for pure-Lua libraries that are loaded
> > using require() from a module that wants them.
> > 
> > ScribuntoExternalLibraries is for libraries that are loaded by default. A
> > library that is implemented with callbacks into PHP code currently needs to
> > be done using this hook, as the require() code path doesn't allow for
> > setting things up.
> > 
> > Since this does appear to be a pure-Lua library that isn't of general
> > utility, ScribuntoExternalLibraryPaths probably would be a good way to go.
> > 
> 
> As we now call out to PHP in order to add the RL module, this is no longer
> be possible for mw.capiunto.Infobox (see below). But I've made
> mw.capiunto.Infobox._render a library which can be dynamically loaded, when
> needed.
I wonder if this tradeoff was worth it. Maybe we should have $wgScribuntoAllowedResourceLoaderModules[] that extensions can add to, and have a Lua method to pull in any listed there. I think we'd be better off doing that if it meant keeping this a pure-Lua library only loaded on demand.
> 
> > includes/lua/Infobox.lua line 129:
> > > data = preprocess( data ),
> > 
> > I see you're preprocessing a lot of the fields being passed in. Note that
> > when the fields are coming in from frame.args, they're already preprocessed
> > and shouldn't need to be preprocessed again; 'data' here seems particularly
> > likely to be in that situation.
> > 
> 
> I don't see a way around doing preprocess on everything right now... the
> problem is that we will get data from various sources and during some
> initial tests it turned out that preprocessing is something people just
> assume to be done. What we *could* do is add specific methods (or flags)
> that bypass (or enable?) preprocessing, but I'm not a huge fan of that.
I can't think of any use case at all where preprocessing like we are now is a good thing. In practice, all data from Lua came from the parser anyway, where we'd see it already preprocessed. I'd favor removing the method altogether.
> 
> > includes/lua/Infobox.lua line 236:
> > > mw.capiunto = mw.capiunto or {}
> > 
> > The evolving style would point to using mw.ext.Capiunto.
> > 
> I'm not a fan of that as it seems overcomplicated. If you insist, I can
> change it, though.
I'd like to see this change as well. I noticed this myself.

From IRC:

<hoo> wgScribuntoAllowedResourceLoaderModules << nope nope nope
<jackmcbarn> why? a module could effectively do that anyway by rendering an infobox but throwing away its content
<hoo> jackmcbarn: Why should it do that? To have the mw-capiunto CSS declarations loaded?
<jackmcbarn> there isn't a good reason for that; i'm just saying it was already possible
<jackmcbarn> why "nope nope nope" is what i'm asking?
<hoo> well, but the idea of having a facility to allow Lua to load modules at all seems scary
<jackmcbarn> yes, but it's no scarier to expose it directly than to expose it indirectly, at least imo
<hoo> Totally disagree
<jackmcbarn> why?
<hoo> let's not make Scribunto the do-it-all box
<hoo> we did that in the past with over extensions and core and that's not especially much fun
<jackmcbarn> the idea is that Capiunto's stylesheet would be the only thing in that, at least for now, and the only future things would be other extensions like Capiunto
<hoo> If there's a need for something like that, we can talk about that again
<hoo> but not if there's only a single extension doing that
<hoo> no need to enter the evil road here
<jackmcbarn> i'm not sure, especially since we can't easily switch from mw.ext.Capiunto to require('capiunto') later if we change our minds. i'll post our convo on the bug and let brad weigh in
<jackmcbarn> what do you think of my other suggestions?
<hoo> jackmcbarn: preprocess... mh
<hoo> not sure the thing is usable without it
<hoo> we could make it an option though
<jackmcbarn> what case can you think of where it would be a good idea to preprocess?
<jackmcbarn> i literally can't think of any
<hoo> Someone was playing with it and a lot of stuff didn't work as expected there
<hoo> that's why I added it
<hoo> I think he recreated some enwiki Infoboxes
<jackmcbarn> but enwiki infoboxes will continue to be fed to a shell around capiunto via wikitext. and frame:preprocess is considered evil, up there next to goto
<hoo> Wikitext and the Parser are awry, sure
<hoo> but I doubt we can get around that
<hoo> still a MediaWiki world
<hoo> and people expect stuff to work "like templates"
<jackmcbarn> before Lua, there was a guarantee that text wouldn't get preprocessed twice
<jackmcbarn> now this will be preprocessing stuff twice, and it could break fragile things
<jackmcbarn> (the reason preprocess is considered evil is that all text in lua should already be preprocessed)
<hoo> I know... but this is supposed to be easy to use and I don't want to completely throw this over
<hoo> also I'm not sure how that will work with content from Wikdiata
<jackmcbarn> does Wikidata provide wikitext content?
<hoo> It can, but it's 5:30am and so I'm not exactly sure how it looks anymore
<jackmcbarn> ok, i'll pick this up some other day then

(In reply to Jackmcbarn from comment #9)
> I wonder if this tradeoff was worth it. Maybe we should have
> $wgScribuntoAllowedResourceLoaderModules[] that extensions can add to, and
> have a Lua method to pull in any listed there. I think we'd be better off
> doing that if it meant keeping this a pure-Lua library only loaded on demand.

I don't much like the idea of adding yet-another config variable. It's probably better to allow non-pure-Lua libraries to be loaded on demand. Gerrit change #163211.


(In reply to Jackmcbarn from comment #10)
> <jackmcbarn> but enwiki infoboxes will continue to be fed to a shell around
> capiunto via wikitext. and frame:preprocess is considered evil, up there
> next to goto
> <hoo> Wikitext and the Parser are awry, sure
> <hoo> but I doubt we can get around that
> <hoo> still a MediaWiki world
> <hoo> and people expect stuff to work "like templates"
> <jackmcbarn> before Lua, there was a guarantee that text wouldn't get
> preprocessed twice
> <jackmcbarn> now this will be preprocessing stuff twice, and it could break
> fragile things
> <jackmcbarn> (the reason preprocess is considered evil is that all text in
> lua should already be preprocessed)
> <hoo> I know... but this is supposed to be easy to use and I don't want to
> completely throw this over
> <hoo> also I'm not sure how that will work with content from Wikdiata

Then the fetch-from-Wikidata should preprocess when necessary. Don't go preprocessing everything that comes in from the parser, double-preprocessing is bad.

Change 163325 had a related patch set uploaded by Jackmcbarn:
Revert "Preprocess text arguments"

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

(In reply to Brad Jorsch from comment #11)
> (In reply to Jackmcbarn from comment #9)
> > I wonder if this tradeoff was worth it. Maybe we should have
> > $wgScribuntoAllowedResourceLoaderModules[] that extensions can add to, and
> > have a Lua method to pull in any listed there. I think we'd be better off
> > doing that if it meant keeping this a pure-Lua library only loaded on demand.
> 
> I don't much like the idea of adding yet-another config variable. It's
> probably better to allow non-pure-Lua libraries to be loaded on demand.
> Gerrit change #163211.
Agreed. This is definitely the way we should go. If we do go this route, we should probably abandon https://gerrit.wikimedia.org/r/#/c/163063/ and just roll _render into Infobox.

Change 163071 merged by jenkins-bot:
Don't inline hard coded CSS

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