Thanks for creating this dedicated task!

Proposed hook design:

• onScribuntoLuaTitleAttributes( LuaEngine $engine, array $resolvers )
  • Each resolver is a function that takes the LuaEngine and a LinkTarget, and returns an arbitrary value to be set as the attribute value.

This is not what your existing patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/1091767 does: it uses the signature onScribuntoLuaTitleAttributes( LuaEngine $engine, LinkTarget $target, string $attribute, mixed &$result ), i.e. it combines what you propose here to be the hook handler and what you propose to be the resolver (and it also uses a by-ref parameter for returning data, which is a must, otherwise it won’t work). I like the hook-only version (what the patch does) better, as it does no unnecessary processing in case no custom attributes are accessed (which is probably the vast majority of module calls), and it also allows for type declarations (callables cannot have further parameter types defined, while the hook itself can have parameter types). Since the returned values are cached in Lua anyway, repeatedly accessing an attribute won’t result in repeated firing of the hook, so separately caching the resolvers doesn’t really make the code more efficient.

• In case the returned php value needs to have some postprocessing done at the Lua side, the resolver returns an array in which a __transformer key is set to the name of a transformer function. The entire array is passed to the transformer function defined on Lua side. Scribunto core will provide some transformers like titleFromText (for the attribute to resolve to an mw.title object) and languageFromName (to resolve to an mw.language object).

As I wrote in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/1091767/comment/144b238f_17d86819/, I think it would be more straightforward if the handler (whether hook handler or transformer) always returned an array, not only when postprocessing is needed.

It will not be possible for extensions to define their own transformers.

While I wouldn’t overthink this in the initial version (all four listed use cases should be fine with returning strings and plain tables), I’d leave this open should such need arise.

In T380690#10351195, @Tacsipacsi wrote:

This is not what your existing patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/1091767 does: it uses the signature onScribuntoLuaTitleAttributes( LuaEngine $engine, LinkTarget $target, string $attribute, mixed &$result ), i.e. it combines what you propose here to be the hook handler and what you propose to be the resolver (and it also uses a by-ref parameter for returning data, which is a must, otherwise it won’t work). I like the hook-only version (what the patch does) better, as it does no unnecessary processing in case no custom attributes are accessed (which is probably the vast majority of module calls), and it also allows for type declarations (callables cannot have further parameter types defined, while the hook itself can have parameter types). Since the returned values are cached in Lua anyway, repeatedly accessing an attribute won’t result in repeated firing of the hook, so separately caching the resolvers doesn’t really make the code more efficient.

I have updated the task description to clarify what I mean.

It was already clear for me (or at least I have an interpretation that sounds clear to me and that hasn’t changed with your update), I just don’t agree with it. Maybe it would help if you wrote down why – and not only how – you think the already-implemented hook needs to be changed.

@Tacsipacsi The current implementation is very imperative in style and overly flexible. I've seen new hooks being added lately tending to prefer a more declarative style, like in this proposal. Here, we have an associative array of resolvers which extensions can add to. The code in the extensions would take the form:

public function onScribuntoLuaAttributes( LuaEngine $engine, array &$resolvers )  {
    $resolvers[ 'isDisambig' ] = function( LuaEngine  $engine, LinkTarget $target ) {
        return checkIfDisambigPage( $target );
    } 
}

which is clean, as opposed to the initial implementation in the patch which requires:

public function onScribuntoLuaAttributes( LuaEngine $engine, LinkTarget $target, string $attribute, mixed &$result )  {
    if ( $attribute !== 'isDisambig' ) {
        return;
    }
    $result = checkIfDisambigPage( $target );
}

Also, in case we have a TitleBatch class in the future (basically T376564), we can extend the same hook to accept a third parameter &batchAttributeResolvers.

Change #1091767 had a related patch set uploaded (by SD0001; author: SD0001):

[mediawiki/extensions/Scribunto@master] Allow extensions to add attributes to mw.title objects

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

Change #1091767 merged by jenkins-bot:

[mediawiki/extensions/Scribunto@master] Allow extensions to add attributes to mw.title objects

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