Page MenuHomePhabricator

In CI explicitly define VisualEditor as a dependency
Open, Needs TriagePublic

Description

There are 35 extensions having a ResourceModules with a dependency upon a module that starts with ext.visualEditor:

AtMentions
BlueSpiceProDistributionConnector
BlueSpiceVisualEditorConnector
Chart
Checklists
Cite
Citoid
CodeMirror
CognitiveProcessDesigner
CollabPads
ContainerFilter
ContentDroplets
ContentTranslation
DateTimeTools
Disambiguator
DiscussionTools
EnhancedUpload
Flow
GrowthExperiments
Kartographer
LanguageTool
Math
PDFCreator
ProofreadPage
Sanctions
Score
SimpleTasks
SoftRedirector
SyntaxHighlight_GeSHi
TEI
Translate
VECancelButton
VEForAll
VisualEditorPlus
wikihiero

They would need VisualEditor to be explicitly defined as a dependency in order to fullfil MediaWiki core test ResourcesTest::testValidDependencies.

I went with a small python script:

parse_extensions
#!/usr/bin/python

from glob import glob
import json
import os.path
import yaml

with open(
    os.path.expanduser('~/projects/integration/config/zuul/dependencies.yaml')
) as f:
    ci_deps = yaml.safe_load(f)

for ext in sorted(glob('*/extension.json')):
    name = os.path.dirname(ext)

    if name == 'VisualEditor':
        continue

    if 'VisualEditor' in ci_deps.get(name, []):
        continue

    with open(ext) as f:
        registry = json.load(f)

    for module_name, module in registry.get('ResourceModules', {}).items():
        if any(
            dep for dep in module.get("dependencies", [])
            if dep.startswith('ext.visualEditor.')
        ):
            print('|', name)
            break

Which gives:

BlueSpiceProDistributionConnector
Chart
CognitiveProcessDesigner
PDFCreator
Sanctions
SimpleTasks

All of them currently get VisualEditor as a transitive dependency with the exception of PDFCreator.

Event Timeline

You may / may-not want to exclude extensions that have a RL module that's defined with a VE dependency but then only uses it in the VisualEditor PluginModules section of the extension.json. That pattern exists to allow an optional dependency where if VE is also installed, code to support it will get loaded.

Testing with VE enabled is handy to exercise more of the extension code and is the only situation that's relevant for WMF-managed wikis, of course.

Change #1193486 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Zuul: add VisualEditor dependency explicitly

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

@DLynch thank you for your comment! I am trying to avoid injecting VisualEditor whenever possible :]

Taking Cite as an example:

  • Cite has an integration with VisualEditor and we certainly want it to be tested. CI should inject it and it is indeed explicitly added in the dependency list. Thus the tests will still be run.
  • CirrusSearch has an integration with Cite, but does not have one with VisualEditor, we thus do not need VE. But since the module is always defined in extension.json mediawiki/core ResourcesTest::testValidDependencies would end up failing (the test validates every single RL dependencies exists).

The way I solved that for some other repositories was to move the resource loader module from extension.json to the ResourceLoaderRegisterModules hook and only define the module when VisualEditor is loaded.

I gave it a try earlier this afternoon with ProofreadPage and Translate:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ProofreadPage/+/1193456
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Translate/+/1193465

So that when an extension depends on ProofreadPage or Translate, they would not need VisualEditor to be injected. Then I am not sure I like that pattern, and maybe it is easier to always inject VisualEditor whenever a dependency needs it.

Change #1193486 merged by jenkins-bot:

[integration/config@master] Zuul: add VisualEditor dependency explicitly

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

Couldn’t an extension.json syntax be invented for “define this RL module only if extension X is loaded” (or a more generic “define this RL module only if callback Y returns true”)? I think it’s common enough, especially for VE integration modules, to warrant not adding these modules in hook handlers: hook handlers require a lot of boilerplate, are harder to test statically (you’d still want to test if dependencies are correct if VE is loaded anyway in the test environment, e.g. when running tests on Translate changes), and are surprising and thus hard to find when one is looking for a module (or tries to get a picture of what modules the extension provides).

I’d still give this a try. I don’t like the hook handler pattern, but I don’t like it either when CI takes too much time on my patch, so optimizing the number of loaded extensions is worth it.

I'd agree that some sort of conditional-definition syntax fills a common need. If you look for usage of PluginModules in extension.json you can see it used to send conditional modules to VisualEditor, CodeMirror, Popups, CollabPads, a bunch of BlueSpice extensions, and other non-WMF extensions.

Change #1194897 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Zuul: [mediawiki/extensions/BlueSpicePageTemplates] add VisualEditor

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

Change #1194898 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Zuul: [mediawiki/extensions/NSFileRepo] add VisualEditor

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

Change #1194899 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Zuul: [mediawiki/extensions/JsonConfig] add VisualEditor

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

Change #1194900 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Zuul: [mediawiki/extensions/MathSearch] add VisualEditor

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

Change #1194901 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] Zuul: [mediawiki/extensions/BlueSpiceDistributionConnector] add VisualEditor

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

Change #1194901 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/BlueSpiceDistributionConnector] add VisualEditor

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

Change #1194897 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/BlueSpicePageTemplates] add VisualEditor

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

Change #1194899 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/JsonConfig] add VisualEditor

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

Change #1194900 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/MathSearch] add VisualEditor

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

Change #1194898 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/NSFileRepo] add VisualEditor

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

Change #1193465 had a related patch set uploaded (by Hashar; author: Hashar):

[mediawiki/extensions/Translate@master] Only load VisualEditor integration when it is loaded

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

Change #1193465 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] Only load VisualEditor integration when it is loaded

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

Change #1217525 had a related patch set uploaded (by Daimona Eaytoy; author: Hashar):

[mediawiki/extensions/Translate@REL1_43] Only load VisualEditor integration when it is loaded

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

Change #1217527 had a related patch set uploaded (by Daimona Eaytoy; author: Hashar):

[mediawiki/extensions/Translate@REL1_44] Only load VisualEditor integration when it is loaded

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

Change #1217529 had a related patch set uploaded (by Daimona Eaytoy; author: Hashar):

[mediawiki/extensions/Translate@REL1_39] Only load VisualEditor integration when it is loaded

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

Change #1217527 merged by jenkins-bot:

[mediawiki/extensions/Translate@REL1_44] Only load VisualEditor integration when it is loaded

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

Change #1217525 merged by jenkins-bot:

[mediawiki/extensions/Translate@REL1_43] Only load VisualEditor integration when it is loaded

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

Change #1217529 merged by Umherirrender:

[mediawiki/extensions/Translate@REL1_39] Only load VisualEditor integration when it is loaded

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