Suppose this is related to T139852

If skins is an empty list, then it means all skins. Using formatversion=2 (https://en.wikipedia.org/w/api.php?action=query&list=gadgets&gaprop=id|metadata&formatversion=2) gives real booleans for the default field.

How does this correspond to what's found here though,

<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=ext.echo.badgeicons%7Cext.echo.styles.badge%7Cext.gadget.WatchlistBase%2CWatchlistGreenIndicators%7Cext.uls.interlanguage%7Cext.visualEditor.desktopArticleTarget.noscript%7Cext.wikimediaBadges%7Cmediawiki.legacy.commonPrint%2Cshared%7Cmediawiki.sectionAnchor%7Cmediawiki.skinning.interface%7Cskins.vector.styles&amp;only=styles&amp;skin=vector"/>

For example,

{
                "id": "featured-articles-links",
                "metadata": {
                    "settings": {
                        "rights": [],
                        "skins": [],
                        "default": true,
                        "hidden": false,
                        "shared": false,
                        "category": "appearance",
                        "legacyscripts": false
                    },
                    "module": {
                        "scripts": [
                            "MediaWiki:Gadget-featured-articles-links.js"
                        ],
                        "styles": [
                            "MediaWiki:Gadget-featured-articles-links.css"
                        ],
                        "dependencies": [],
                        "peers": [],
                        "messages": []
                    }
                }
            },

doesn't appear in ext.gadget.WatchlistBase%2CWatchlistGreenIndicators

The Parse API by design only exposes modules added through Parser/ParserOutput. Modules added by OutputPage or Skin (or extensions using hooks at that level instead of at parser level) are not exposed.

I tried to fix this as part of T130632: mediawiki.toc module unnecessarily shipped in mobile views:

• rMW90c95fc7f290: ApiParse: Add Skin::getDefaultModules() in useskin mode
• rMWdf5b122641bf: ApiParse: Add 'useskin' param for general OutputPage/Skin support
• rMWf19050377c7b: ApiParse: Deprecate prop=headitems
• rMW9093af0a2874: Remove modulemessages from ApiParse and Output (deprecated in 1.26)

The main one here is df5b12264, which adds a useskin parameter to the Parse API that will pull the ParserOutput through OutputPage and Skin, ensuring that any default modules or skin-specific modules get queued as well.

For example:

https://sv.wikipedia.org/w/api.php?action=parse&page=Mir&prop=modules|jsconfigvars

"modules": [
    "ext.cite.a11y"
],
"modulescripts": [],
"modulestyles": [
    "ext.cite.styles"
],

https://sv.wikipedia.org/w/api.php?action=parse&page=Mir&prop=modules|jsconfigvars&useskin=vector

"modules": [
    "ext.cite.a11y",
    "site",
    "mediawiki.page.startup",
    "mediawiki.user",
    "mediawiki.hidpi",
    "mediawiki.page.ready",
    "mediawiki.toc",
    "mediawiki.searchSuggest",
    "mediawiki.page.watch.ajax"
],
"modulescripts": [],
"modulestyles": [
    "ext.cite.styles"
],

However, it seems gadgets are still not included. Not sure why..

However, it seems gadgets are still not included. Not sure why..

Figured out the reason. Parse API applied OutputPage::addParserOutputMetadata() (which also runs the OutputPageParserOutput hook), and applies Skin::getDefaultModules().

However, Gadgets (like many other extensions) use the BeforePageDisplay hook to queue modules, which is only run from OutputPage::output() – which makes semantically sense, and shouldn't be run in API context probably.

On the other hand, it's unfortunate that lots of extensions have started to use BeforePageDisplay as a very generic hook to do pretty much anything (which is more reason not to run it in API context). It's not an easy fix, but I'd rather deprecate using BeforePageDisplay to add modules and instead come up with a better way. Perhaps OutputPageParserOutput makes more sense, but that hook has two problems:

1. It's primarily intended to perform actions that relate to the ParserOutput object, not generic unconditional modules added on every page, or things specific to the current user or skin. In addition, extensions should still be able to unconditionally queue a module in a way that also applies to special pages (e.g. EventLogging).
2. OutputPageParserOutput may run multiple times since you can add multiple parser outputs to a page (which is in fact a common thing to do on special pages).

So with that, BeforePageDisplay actually makes a lot of sense. It's run only once, relates to page rendering, and has access to the skin.

So perhaps we should consider running it in Parse API (when the "useskin" mode is enabled). Do we any usage of this hook that would cause problems in API context?

@Anomie Thoughts?

Do we any usage of this hook that would cause problems in API context?

That's the important question. There are a lot of users of that hook, someone would have to look through them all.

I looked at https://sv.wikipedia.org/wiki/Mir?veaction=editsource in the 2017WTE preview (Parsoid) and the saved page. While the two aren't absolutely identical (e.g., the white space in the infobox is slightly different), I didn't see anything that actually looked broken. Is that expected, or has this problem somehow gotten better since this task was created ~16 months ago?

Strawman: what if we (a) deprecate using BeforePageDisplay to add modules, but (b) add a new hook (AddParserModules?) which is run at basically the same time, but which is also run during the Parse API when useskin is enabled. That allows us to incrementally migrate users of BeforePageDisplay as a module-hook, without risking breakage by running BeforePageDisplay in the ParseAPI where existing callers may not expect.

Change 460406 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/core@master] WIP: hard-deprecate adding modules from BeforePageDisplay hook

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

The other thing the Gadget extension does in the BeforePageDisplay hook is add warnings about using legacy APIs as raw HTML concatenated to the end of the display page. This is the sort of thing we probably *don't* want to expose/allow in the Parse API?

https://github.com/wikimedia/mediawiki-extensions-Gadgets/blob/master/includes/GadgetHooks.php#L222

So we'd split the hook in two; the ExtraPageModules hook would iterate through the gadgets and add the styles and modules, and then the BeforePageDisplay hook would just add the warning text at the bottom.

Since adding modules should be idempotent, extensions which want to migrate to the new hook structure could invoke the ExtraPageModules hook function explicitly at the top of BeforePageDisplay, and we can be sure that that is 'safe' (ie, doesn't emit deprecation warnings, the redundant modules are ignored).

Change 460422 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/extensions/Gadgets@master] WIP: Use new ExtraPageModules hook

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

@Krinkle, @Legoktm does the above plan seem reasonable? there's a WIP patch in gerrit, but I don't want to develop it further before getting signoff on the theory.

EDIT: nevermind, I see discussion has started on https://gerrit.wikimedia.org/r/460406

Other users of the BeforePageDisplay hook, via codesearch:

• AdvancedMeta, for example
  • Uses: $out->addJsConfigVars(), $out->getTitle()
  • Don't move to new hook: $out->addMeta(), $out->setIndexPolicy(), $out->setHTMLTitle()
• Wikibase, for example
  • Uses: $out->addJsConfigVars(), $out->getProperty( 'wikibase_item' ), $skin, $out->addModules()
• CentralAuth, for example
  • Uses: $out->addJsConfigVars(). $out->addModules()
  • Don't move: $out->addHeadItem(), $out->addHTML(), $out->getRequest()->getSessionData()
• ContentTranslation, for example
  • Uses: $title, $user, $out->getRequest()->getCookie(), $out->addModules(), Action::getActionName( $out->getContext() )
• Echo, for example
  • Uses: $user, $skin->getSkinName(), $out->addModules(), $out->addModuleStyles()
• EventLogging, for example
  • Uses: $out->addModules(), $user, $out->getRevisionId(), $out->addSubtitle() (don't move this one?)
• MobileFrontend, for example
  • Don't move? $out->addLink(), $out->addVaryHeader(), etc
• MultiLanguageManager, for example
  • usual stuff, jsvars, modules, modulestyles, $title
• ORES, for example
  • Uses: $out->getProperty( 'oresData' ), Helpers::isHighlightEnabled( $out ) ($user/$title, aka $context)
• PageImages, for example
  • Don't move: $out->addMeta()

... ran out of energy to review codesearch results at this point.

There's also a BeforePageDisplayMobile hook...

Note that, in my experience, most gadgets relate to the site interface and/or to JS-based functionality added to the page; rarely is a gadget a styles-only module targetting wikitext-rendered content.

I considered a while back about whether it'd make sense to indicate in some way in the gadget definition whether we should queue it to ParserOutput (for content / API consumption) vs OutputPage (for skinned page loads only). However, this has since been superseded by TemplateStyles.

It seem that this bug partly explains why ZIM files of Wiktionary in English are crappy, see https://github.com/openzim/mwoffliner/pull/1665

In T161278#4593665, @cscott wrote:

@Krinkle, @Legoktm does the above plan seem reasonable?

Yes.

Change 460406 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] WIP: hard-deprecate adding modules from BeforePageDisplay hook

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

Change 460422 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/extensions/Gadgets@master] WIP: Use new ExtraPageModules hook

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

I considered a while back about whether it'd make sense to indicate in some way in the gadget definition whether we should queue it to ParserOutput (for content / API consumption) vs OutputPage (for skinned page loads only). However, this has since been superseded by TemplateStyles.

I think this is probably true for best/modern practices. We currently have a large fleet that doesn't adhere to those good practices due to old use, and there are also third parties that won't use TemplateStyles for whatever reason (one user wanted help with a problem [when I originally wrote this] in CSS and a suggestion to use TemplateStyles did not go over well due to it not being in the core software - yeah, I know). I don't know if the second group merits serious consideration here, but we do need to give serious consideration to the first.

That's besides more broad changes like en.wp's restyling of <cite>, <sup>, and the like in Common.css which clearly target the content and which cannot feasibly be targeted by TemplateStyles.

(I'm not sure exactly what implementation plan I'm arguing for/against here, just adding input.)