Page MenuHomePhabricator

Cargo's Lua support causing errors
Open, Needs TriagePublicBUG REPORT

Description

hi! i'm trying to make a module where every five parameters makes a new role in a table. however, when trying to store these data with Cargo, it seems to break entirely, causing internal errors.

the snippet on mediawiki's site (https://www.mediawiki.org/wiki/Extension:Cargo/Other_features#Lua_support) causes an error in the module, and a workaround hack by wrapping the names in [''] doesnt seem to work too, causing an internal error

→ module: https://tagging.wiki/wiki/Module:RoleTable
→ sandbox, internal error zone: https://tagging.wiki/wiki/User:Splatched/sandbox

Event Timeline

Backtrace from @SomeRandomDeveloper in https://issue-tracker.miraheze.org/P567

CargoLuaLibrary::cargoDeclare(): Argument #1 ($args) must be of type array, string given, called in /srv/mediawiki/1.44/extensions/Scribunto/includes/Engines/LuaSandbox/LuaSandboxCallback.php on line 31

from /srv/mediawiki/1.44/extensions/Cargo/includes/CargoLuaLibrary.php(151)
#0 /srv/mediawiki/1.44/extensions/Scribunto/includes/Engines/LuaSandbox/LuaSandboxCallback.php(31): CargoLuaLibrary->cargoDeclare(string)
#1 [internal function]: MediaWiki\Extension\Scribunto\Engines\LuaSandbox\LuaSandboxCallback->__call(string, array)
#2 /srv/mediawiki/1.44/extensions/Scribunto/includes/Engines/LuaSandbox/LuaSandboxInterpreter.php(137): LuaSandboxFunction->call(LuaSandboxFunction)
#3 /srv/mediawiki/1.44/extensions/Scribunto/includes/Engines/LuaCommon/LuaEngine.php(312): MediaWiki\Extension\Scribunto\Engines\LuaSandbox\LuaSandboxInterpreter->callFunction(LuaSandboxFunction, LuaSandboxFunction)
#4 /srv/mediawiki/1.44/extensions/Scribunto/includes/Engines/LuaCommon/LuaModule.php(76): MediaWiki\Extension\Scribunto\Engines\LuaCommon\LuaEngine->executeFunctionChunk(LuaSandboxFunction, MediaWiki\Parser\PPTemplateFrame_Hash)
#5 /srv/mediawiki/1.44/extensions/Scribunto/includes/Hooks.php(196): MediaWiki\Extension\Scribunto\Engines\LuaCommon\LuaModule->invoke(string, MediaWiki\Parser\PPTemplateFrame_Hash)
#6 /srv/mediawiki/1.44/includes/parser/Parser.php(3492): MediaWiki\Extension\Scribunto\Hooks->invokeHook(MediaWiki\Parser\Parser, MediaWiki\Parser\PPTemplateFrame_Hash, array)
#7 /srv/mediawiki/1.44/includes/parser/Parser.php(3147): MediaWiki\Parser\Parser->callParserFunction(MediaWiki\Parser\PPTemplateFrame_Hash, string, array, bool)
#8 /srv/mediawiki/1.44/includes/parser/PPFrame_Hash.php(280): MediaWiki\Parser\Parser->braceSubstitution(array, MediaWiki\Parser\PPTemplateFrame_Hash)
#9 /srv/mediawiki/1.44/includes/parser/Parser.php(3348): MediaWiki\Parser\PPFrame_Hash->expand(MediaWiki\Parser\PPNode_Hash_Tree)
#10 /srv/mediawiki/1.44/includes/parser/PPFrame_Hash.php(280): MediaWiki\Parser\Parser->braceSubstitution(array, MediaWiki\Parser\PPFrame_Hash)
#11 /srv/mediawiki/1.44/includes/parser/Parser.php(2970): MediaWiki\Parser\PPFrame_Hash->expand(MediaWiki\Parser\PPNode_Hash_Tree, int)
#12 /srv/mediawiki/1.44/includes/parser/Parser.php(1606): MediaWiki\Parser\Parser->replaceVariables(string)
#13 /srv/mediawiki/1.44/includes/parser/Parser.php(705): MediaWiki\Parser\Parser->internalParse(string)
#14 /srv/mediawiki/1.44/includes/content/WikitextContentHandler.php(380): MediaWiki\Parser\Parser->parse(string, MediaWiki\Title\Title, MediaWiki\Parser\ParserOptions, bool, bool, int)
#15 /srv/mediawiki/1.44/includes/content/ContentHandler.php(1743): MediaWiki\Content\WikitextContentHandler->fillParserOutput(MediaWiki\Content\WikitextContent, MediaWiki\Content\Renderer\ContentParseParams, MediaWiki\Parser\ParserOutput)
#16 /srv/mediawiki/1.44/includes/content/Renderer/ContentRenderer.php(75): MediaWiki\Content\ContentHandler->getParserOutput(MediaWiki\Content\WikitextContent, MediaWiki\Content\Renderer\ContentParseParams)
#17 /srv/mediawiki/1.44/includes/Revision/RenderedRevision.php(260): MediaWiki\Content\Renderer\ContentRenderer->getParserOutput(MediaWiki\Content\WikitextContent, MediaWiki\Page\PageIdentityValue, MediaWiki\Revision\RevisionStoreRecord, MediaWiki\Parser\ParserOptions, array)
#18 /srv/mediawiki/1.44/includes/Revision/RenderedRevision.php(233): MediaWiki\Revision\RenderedRevision->getSlotParserOutputUncached(MediaWiki\Content\WikitextContent, array)
#19 /srv/mediawiki/1.44/includes/Revision/RevisionRenderer.php(236): MediaWiki\Revision\RenderedRevision->getSlotParserOutput(string, array)
#20 /srv/mediawiki/1.44/includes/Revision/RevisionRenderer.php(169): MediaWiki\Revision\RevisionRenderer->combineSlotOutput(MediaWiki\Revision\RenderedRevision, MediaWiki\Parser\ParserOptions, array)
#21 /srv/mediawiki/1.44/includes/Revision/RenderedRevision.php(196): MediaWiki\Revision\RevisionRenderer->MediaWiki\Revision\{closure}(MediaWiki\Revision\RenderedRevision, array)
#22 /srv/mediawiki/1.44/includes/page/ParserOutputAccess.php(458): MediaWiki\Revision\RenderedRevision->getRevisionParserOutput()
#23 /srv/mediawiki/1.44/includes/page/ParserOutputAccess.php(368): MediaWiki\Page\ParserOutputAccess->renderRevision(MediaWiki\Page\WikiPage, MediaWiki\Parser\ParserOptions, MediaWiki\Revision\RevisionStoreRecord, int, null)
#24 /srv/mediawiki/1.44/includes/page/WikiPage.php(1132): MediaWiki\Page\ParserOutputAccess->getParserOutput(MediaWiki\Page\WikiPage, MediaWiki\Parser\ParserOptions, MediaWiki\Revision\RevisionStoreRecord, int)
#25 /srv/mediawiki/1.44/includes/page/Article.php(2038): MediaWiki\Page\WikiPage->getParserOutput(MediaWiki\Parser\ParserOptions, int)
#26 /srv/mediawiki/1.44/extensions/ProtectionIndicator/includes/ProtectionIndicatorHooks.php(60): MediaWiki\Page\Article->getParserOutput(int)
#27 /srv/mediawiki/1.44/includes/HookContainer/HookContainer.php(155): MediaWiki\Extension\ProtectionIndicator\ProtectionIndicatorHooks::onArticleViewHeader(MediaWiki\Page\Article, null, bool)
#28 /srv/mediawiki/1.44/includes/HookContainer/HookRunner.php(876): MediaWiki\HookContainer\HookContainer->run(string, array)
#29 /srv/mediawiki/1.44/includes/page/Article.php(708): MediaWiki\HookContainer\HookRunner->onArticleViewHeader(MediaWiki\Page\Article, null, bool)
#30 /srv/mediawiki/1.44/includes/page/Article.php(551): MediaWiki\Page\Article->generateContentOutput(MediaWiki\User\User, MediaWiki\Parser\ParserOptions, int, MediaWiki\Output\OutputPage, array)
#31 /srv/mediawiki/1.44/includes/actions/ViewAction.php(80): MediaWiki\Page\Article->view()
#32 /srv/mediawiki/1.44/includes/actions/ActionEntryPoint.php(728): MediaWiki\Actions\ViewAction->show()
#33 /srv/mediawiki/1.44/includes/actions/ActionEntryPoint.php(505): MediaWiki\Actions\ActionEntryPoint->performAction(MediaWiki\Page\Article, MediaWiki\Title\Title)
#34 /srv/mediawiki/1.44/includes/actions/ActionEntryPoint.php(143): MediaWiki\Actions\ActionEntryPoint->performRequest()
#35 /srv/mediawiki/1.44/includes/MediaWikiEntryPoint.php(202): MediaWiki\Actions\ActionEntryPoint->execute()
#36 /srv/mediawiki/config/initialise/entrypoints/index.php(71): MediaWiki\MediaWikiEntryPoint->run()
#37 {main}

IMO avoid a pattern where you invoke a table declare on each module call. You'll end up with a potentially fairly nasty moment "if" (more on that later) the module gets called on multiple template pages (or a doc page), and in normal circumstances invoking cargo_declare on a non-template page is an error (more on that later in your scenario). If there's an accompanying template (which you sure have given the namespace requirement), it'll be much cleaner if you just call cargo_declare on your own right on the template page.

Now, as to why the error is happening in your case, mw.ext.cargo.declare expects only a single parameter. This parameter should be a table, and matching exactly what you'd have in a regular #cargo_declare wikitext callsite - _table field with your table name, and additional fields for columns. Change that (and REALLY get that declare call out of your articles and only into your primary template), exception will be gone.


So, to unpack the issues in Cargo that this single task has uncovered:

  1. There's a documentation error around the arguments mw.ext.cargo.declare accepts, which is the direct cause of you encountering the exception. (Correcting the doc page in a second.)
    • The parameter in cargo.lua is also misnamed, but that's beside the point.
  2. Error output is suppressed as the native Lua callbacks do not return anything.
  3. Type checks done in PHP are potentially largely redundant, but for sure safe to keep. Function names are mismatched though, so in case of an error it will appear that query was called when it was declare or store.
  4. There is apparently no type-checking in the Lua wrapper while the native function has strict parameter types. This allows invalid data to get through.
    • I would risk saying the API choice was a bad idea in the first place, but that will be a hard thing to fix now that the wikitext-esque invocation style has been in the wild for a while. The idea that the table name will be a separate argument is completely reasonable, as this is supposed to be a "native" interface of invoking Cargo from Lua, and not a dirty alternative to frame:callParserFunction.
  5. There are no type-checks on the table fields, but both the keys and the values should all be strings. This may lead to other possible type mismatch exceptions.
  6. The Lua function invokes CargoDeclare::declareTable directly. This is horrible, because while the function isn't private, until the addition of the Lua functions it is not called outside of CargoDeclare::run anywhere in the project (have not checked other extensions from the developer). CargoDeclare::run is the entrypoint for the wikitext function and among the usual things (like parsing named parameters) it also performs some fix-ups (stripping excess white-space from the input) and validation (aforementioned namespace check!!!).
    • This means that you could cause a situation where a regular article is /trying/ to declare a table.
    • This will also emit the necessary metadata into page_props.
    • Throughout the project there's an assumption that ALL user-tables are defined by templates, and this function allows that assumption to be violated, and in the best-case scenario all this will cause is just more errors.

Will send patches for (2) and (6) in the next 24h, possibly (4) and (5) but I'll need to go through some existing code from other extensions to see how that's handled.

i've removed the cargo.declare from the module and the exception is gone. thanks!

@Alex44019 - thank you for the analysis, and for the fixes/improvements to the documentation. I look forward to any patches you provide.

Change #1216145 had a related patch set uploaded (by Alex44019; author: Alex44019):

[mediawiki/extensions/Cargo@master] CargoDeclare: Perform the namespace check inside `declareTable`

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

Change #1216144 had a related patch set uploaded (by Alex44019; author: Alex44019):

[mediawiki/extensions/Cargo@master] LuaLibrary: In `store`, remove PHP argument type hints and indicate correct function on type error

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

Change #1216143 had a related patch set uploaded (by Alex44019; author: Alex44019):

[mediawiki/extensions/Cargo@master] LuaLibrary: In `declare`, improve argument handling and forward impl. output

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

I think these three address the issues around mw.ext.cargo.declare, and a possible TypeError on mw.ext.cargo.store. Still, .store could benefit from some type checking improvements (check if literal then type-convert to string) as previously there's been a number of issues resulting from this not being done, so this task shouldn't be closed just yet. (I can submit a patch probably some time during the week). Other decent improvement would be throwing inside of .declare on an error rather than requiring that the user includes the output (which may be a null) in their function's output.

Also sent a few other minor fixes in separate changesets (not tagged with this task).

Change #1216143 merged by jenkins-bot:

[mediawiki/extensions/Cargo@master] LuaLibrary: In `declare`, improve argument handling and forward impl. output

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

Change #1216144 merged by jenkins-bot:

[mediawiki/extensions/Cargo@master] LuaLibrary: In `store`, remove PHP argument type hints and indicate correct function on type error

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

Change #1216145 merged by jenkins-bot:

[mediawiki/extensions/Cargo@master] CargoDeclare: Perform the namespace check inside `declareTable`

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