Is there any reason this task needs to be private?

I note that the skin seems to have quite a few dependancies, including the unsupported composer.json require of MW (see partially T194878: Add the Packagist webhook to wikimedia/mediawiki repo on GitHub)... And so do some of the dependancies...

$ composer update --no-dev
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - The requested package mediawiki/mediawiki could not be found in any version, there may be a typo in the package name.
  Problem 2
    - Installation request for mediawiki/bootstrap ^4.0 -> satisfiable by mediawiki/bootstrap[4.0].
    - mediawiki/bootstrap 4.0 requires mediawiki/mediawiki >=1.27.0 -> no matching package found.

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://getcomposer.org/doc/04-schema.md#minimum-stability> for more details.
 - It's a private package and you forgot to add a custom repository to find it

Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.
Running update with --no-dev does not mean require-dev is ignored, it just means the packages will not be installed. If dev requirements are blocking the update you have to resolve those problems.

So it's not installable as is...

In T246949#5957016, @Reedy wrote:

Is there any reason this task needs to be private?

No.

@Jcross the Chameleon skin is ready for review. We are not currently planning to make any changes to Chameleon for this project.

The contractor working on this project for us will be developing other components that will be submitted for security review as they are completed. Additional information about the components is at Technical Docs. But, Chameleon itself does not depend in any way upon these other components.

Hi @CCicalese_WMF - thanks for the update. We'll take a look and please note that at least for deploy, our guidelines require that it is on Gerrit. We'll be in contact as our review proceeds.

In T246949#6041820, @Jcross wrote:

Hi @CCicalese_WMF - thanks for the update. We'll take a look and please note that at least for deploy, our guidelines require that it is on Gerrit. We'll be in contact as our review proceeds.

^ Gerrit will indeed be needed for WMF deploy. I don't know how the sync is going to be done going forward, as the canonical repo is still going to be github etc

Dependancies are not currently deployable in MediaWiki-Vendor as is. It's hard to say it's ready to review if I can't even install anything in our environment (as mentioned before), and therefore isn't particularly "ready to review"

$ git diff
diff --git a/composer.json b/composer.json
index 94c4781d..7466112a 100644
--- a/composer.json
+++ b/composer.json
@@ -42,6 +42,8 @@
                "league/uri-components": "2.2.1",
                "league/uri-interfaces": "2.1.0",
                "liuggio/statsd-php-client": "1.0.18",
+               "mediawiki/bootstrap": "^4.0",
+               "mediawiki/mw-extension-registry-helper": "^1.0",
                "monolog/monolog": "1.25.3",
                "mustangostang/spyc": "0.6.3",
                "nikic/php-parser": "4.3.0",
$ composer update --no-dev
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for mediawiki/bootstrap ^4.0 -> satisfiable by mediawiki/bootstrap[4.0].
    - mediawiki/bootstrap 4.0 requires mediawiki/mediawiki >=1.27.0 -> no matching package found.

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://getcomposer.org/doc/04-schema.md#minimum-stability> for more details.
 - It's a private package and you forgot to add a custom repository to find it

Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.
Running update with --no-dev does not mean require-dev is ignored, it just means the packages will not be installed. If dev requirements are blocking the update you have to resolve those problems.

We also can't install it in prod using https://github.com/ProfessionalWiki/chameleon/blob/HEAD/docs/installation.md#installation - We don't use composer.local.json. Never mind installation of skins/extensions via composer is not supported by MW generally, never mind in WMF prod. And I doubt we want to start doing this for a one off when we didn't do this for Wikidata

Adding it to vendor/composer.json similarly doesn't work, even with "mediawiki/chameleon-skin" in it.

As the skin also depends on bootstrap (among others), I'd highly advice getting Performance-Team to do a perf review too as part of the pre-deploy processes

In T246949#6041968, @Reedy wrote:

As the skin also depends on bootstrap (among others), I'd highly advice getting Performance-Team to do a perf review too as part of the pre-deploy processes

This is as per https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Preparing_for_deployment, https://www.mediawiki.org/wiki/Wikimedia_Performance_Team/Performance_Review and therefore https://phabricator.wikimedia.org/maniphest/task/edit/form/71/ needs filling in

The name of mediawiki/mw-extension-registry-helper got me interested, so I took a look and I don't think we can deploy it, given that its whole goal is to work around the ExtensionRegistry. Specifically T152263, which got stalled in Gerrit because of some legitimate concerns during CR. Loading through a subregistry means that dependency checking doesn't work and then needs to abuse Reflection to get the proper metadata set in the main registry.

Especially for Wikimedia deployment, I think all dependencies should be explicitly loaded in CommonSettings.php rather than subtly through this mechanism.

Thanks for the info, @Reedy and @Legoktm. I will discuss those issues with the Chameleon developer and report back here as well as arranging for a performance review.

It'd be nice to replace mediawiki/mw-extension-registry-helper provides with something simpler while retaining automatic enabling of dependencies. Can an extension somehow enable other extensions that it depends on when it is enabled? (We'd make this optional so you can define things manually/explicitly for WMF wikis.)

I looked into this briefly several months ago and did not find a way. Which is why in Semantic Bundle I just have composer include a file with wfLoadExtensions() since in that package it does not really matter that "don't auto-enable installed packages" gets violated. This won't fly for Chameleon and co though.

(I do not have the time to click though 25 levels of phabricatorception and read various discussions from 2016.)

In T246949#6048314, @JeroenDeDauw wrote:

It'd be nice to replace mediawiki/mw-extension-registry-helper provides with something simpler while retaining automatic enabling of dependencies. Can an extension somehow enable other extensions that it depends on when it is enabled? (We'd make this optional so you can define things manually/explicitly for WMF wikis.)

I looked into this briefly several months ago and did not find a way. Which is why in Semantic Bundle I just have composer include a file with wfLoadExtensions() since in that package it does not really matter that "don't auto-enable installed packages" gets violated. This won't fly for Chameleon and co though.

(I do not have the time to click though 25 levels of phabricatorception and read various discussions from 2016.)

In theory it could call wfLoadExtension() but this seems messy, as well as potentially undefined behaviour. And also not obvious as to why something might be loaded on a wiki, when it's buried in some file inside an extension/skin. But no, we don't support that. For Wikimedia config, we explicitly load extensions as needed; only composer included libraries are just automatically loaded by inclusion in the MediaWiki-Vendor repository for deployment

In theory it could call wfLoadExtension()

I think that is the first thing I tried in Semantic Bundle. If you call wfLoadExtension() for a dependency after the extension you are in got enabled, it is too late. And if you do it beforehand, you end up with the "enabled as soon as installed" problem.

For Wikimedia config, we explicitly load extensions as needed

Yes that is very clear. No one is suggesting you change this. And having a way to automatically load needed extensions does not force WMF to use that. It also does not mean every extension will end up with 20 hard to trace dependencies. I think Chameleon with its usage of the Bootstrap extension is an exception.

So several options here

1. We drop automatic enabling of the dependencies of Chameleon
2. We keep what is currently there but enable turning it off via config
3. We make Chameleon auto enable its dependencies in a better way
4. We make the dependencies (mw-bootstrap) auto enable itself
5. We turn the dependencies into normal libraries and sidestep the enabling question
6. ?

While option 1 is nice and all for WMF it is not good for basically all existing users, so is not really on the table at the moment. I'm not a fan of option 2 since it retains all current complexity and suspect this is also not good from WMF side. Hence I invite help with the other options.

Option 5 seems possible for mw-scss, leaving only mw-bootstrap to deal with. Maybe 5 is also possible for that one, though I suspect putting resource modules somewhere in vendor causes some problems. Self-enabling (4) could be done for mw-boostrap as compromise but it is certainly not ideal, which leaves us with option 3: finding a better way to enable it automatically from Chameleon.

Noting also, we're going to end up with a "fork" of Chameleon on Gerrit anway. Because we don't deploy code from github

Another thing is adding phan to the Skin.... Which while doesn't actually seem to be documented, it's considered best practice and has been dnoe for MW bundled skins and extensions, and many WMF deployed extensions T179554: Add phan to MediaWiki extensions and skins for static analysis [cloneable]

Composer version 3.0 and Bootstrap version 4.2 may now be installed without composer. Thank you to @JeroenDeDauw for working with me to get those changes merged into the skin/extension. The former SCSS extension used by Bootstrap is now a library. The mediawiki/mediawiki and mediawiki/mw-extension-registry-helper packages are no longer used by Chameleon or Bootstrap. The code now conforms to MediaWiki coding guidelines checked by PHP Code Sniffer. Chameleon no longer auto-enables Bootstrap, so the pair must be enabled with

wfLoadExtension( 'Bootstrap' );
wfLoadSkin( 'Chameleon' )'

I could use guidance on the best way to mirror the github repos in gerrit. It would be unfortunate and difficult to sustain a fork, which would not easily benefit from future enhancements, although I respect that it may be necessary to have a human in the loop when importing changes from upstream. Is there a precedent for this?

I had trouble running phan locally, so I will wait until the repo is in gerrit before adding phan to the repo.

This is great to hear.

In T246949#6120275, @CCicalese_WMF wrote:

I could use guidance on the best way to mirror the github repos in gerrit. It would be unfortunate and difficult to sustain a fork, which would not easily benefit from future enhancements, although I respect that it may be necessary to have a human in the loop when importing changes from upstream. Is there a precedent for this?

Not really. Usually, gerrit becomes the canonical, but that obviously doesn't work everywhere. We obviously need to be concious of changes in either direction; we obviously can't expect for Wikimedia production to have to merge commits into the github repo, then pull them over, especially in case of major production issues. I think this is probably a bigger discussion to have with Release-Engineering-Team and should probably be a different task

For the phan thing, and getting things started, we probably want to un-archive https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/skins/chameleon and import (probably temporarily enable force push/forge committer etc) the missing history. I believe the history should be the same, it doesn't even look to have been archived - https://gerrit.wikimedia.org/g/mediawiki/skins/chameleon

You could then at least enable CI for the extension, and start phan development in gerrit etc. Could then be merged into gerrit and pulled to github, or cherry picked from the gerrit change

@Jcross Now that the comments above regarding the Chameleon and Bootstrap installation code have been addressed, can you please move forward on the security review? Note that I have created subtasks for the performance review and determining the relationship between gerrit and github, but those issues should not block security review of the code. If there are any other issues with the code, we would need to know of them soon, so we can address them before deployment. Thanks!

I've just looked there's no stable release yet with the required fixes; the latest 2.3.0 definitely doesn't https://github.com/ProfessionalWiki/chameleon/blob/2.3.0/composer.json

Your patches are still in the diff between 2.3.0...master https://github.com/ProfessionalWiki/chameleon/compare/2.3.0...master

Any idea when we'll have a tagged release as such?

There is a pending pull request for the release. I've inquired as to the release date and will report back here when I know.

In T246949#6133651, @Reedy wrote:

Any idea when we'll have a tagged release as such?

Chameleon 3.0.0 was released on May 20. We will need a subsequent point release for some enhancements such as phan support.

Cindy: What do we want to do with this task? Obviously, Chameleon et al is not relevant for us, and WikimediaApiPortal is kind of a seperate (whilst still related) request

Excellent question. I was getting ready to create new tasks for the security readiness review and performance review for the WikimediaApiPortalOAuth extension and the corresponding new endpoints in the OAuth extension. I think there are three options for the security readiness reviews:

1. Decline this task and open two new tasks: one for WikimediaApiPortal and one for WikimediaApiPortalOAuth/OAuth
2. Decline this task and open a single new task for all of WikimediaApiPortal, WikimediaApiPortalOAuth, and OAuth
3. Rename this task into WikimediaApiPortal and create a new task for WikimediaApiPortalOAuth/OAuth
4. Rename this task into a single task for all of the above

Which would be preferable for you all? Or is there another option I hadn't considered?

For the performance review task, the task already morphed into the review of WikimediaApiPortal, so I think it just makes sense to rename that task and create a separate task for performance review of WikimediaApiPortalOAuth/OAuth.

@Reedy This should now be ready for review.

In T246949#6256725, @WDoranWMF wrote:

@Reedy This should now be ready for review.

Someone not in perf/sec should be reviewing https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/WikimediaApiPortal/+/605825/ (which has some minor issues flagged), and merging it to consider it "ready" :)

Two immediate questions from Installation.md

## Recommended configuration

	// Needed (TODO: Document why).
	$wgUseMediaWikiUIEverywhere = true;

	// To enable a complex Main Page
	$wgRawHtml = true;

Are we wanting/needing $wgRawHtml to be true?

Currently, only a few wikis have it enabled

'wgRawHtml' => [
	'default' => false,
	'donatewiki' => true,
	'internalwiki' => true,
	'collabwiki' => true, // T31269
],

To quote DefaultSettings.php

/**
 * Allow raw, unchecked HTML in "<html>...</html>" sections.
 * THIS IS VERY DANGEROUS on a publicly editable site, so USE $wgGroupPermissions
 * TO RESTRICT EDITING to only those that you trust
 */
$wgRawHtml = false;

Also, why $wgUseMediaWikiUIEverywhere? We don't set it on any WMF wikis atm, though MobileFrontend seemingly does on the fly in some cases...

/**
 * Temporary variable that applies MediaWiki UI wherever it can be supported.
 * Temporary variable that should be removed when mediawiki ui is more
 * stable and change has been communicated.
 * @since 1.24
 */
$wgUseMediaWikiUIEverywhere = false;

In T246949#6268038, @Reedy wrote:

Also, why $wgUseMediaWikiUIEverywhere? We don't set it on any WMF wikis atm, though MobileFrontend seemingly does on the fly in some cases...

/**
 * Temporary variable that applies MediaWiki UI wherever it can be supported.
 * Temporary variable that should be removed when mediawiki ui is more
 * stable and change has been communicated.
 * @since 1.24
 */
$wgUseMediaWikiUIEverywhere = false;

Filed T256767: Update docs about $wgUseMediaWikiUIEverywhere about that one, it might not be needed anymore

(this includes other stuff I've noticed while poking around. Feel free to fork into seperate tasks)

Slight oddity:

default.mustache has random </body></html> at the end, which don't match anything. Even if the opening ones are brought in programatically via some other part of the template, it feels a bit odd to be closing them manually... But that's maybe just how these things are done

Bootstrap 4.3.1 is the latest version of the 4.3 branch, but 4.5.0 is the latest release of the 4.x branch.

https://github.com/twbs/release says 4.x is the active LTS, going into Maintenance LTS on 2020-05-26 and is due to be EOL before the year is out on 2020-11-26.

Bootstrap 5 is in alpha, and due for release late this year.

What is the maintenance plan for keep this relatively up to date (at least, as far as keeping on "supported" versions)?

The requirement for "composer/installers": "~1.0" should be removed, AIUI?

		// Ignore wgLogo and 'logopath' from SkinTemplate
		$stylePath = $this->getSkin()->getConfig()->get( 'StylePath' );
		$logoPath = $stylePath . '/WikimediaApiPortal/resources/images/icon/wikimedia-black.svg';

Why are we loading the logo from the extension path?

It feels odd having this wiki as a special case, considering the rest use wgLogo, and load the logos from https://en.wikipedia.org/static/images/project-logos/enwiki-2x.png... Maybe something more for T252462: Performance review of WikimediaApiPortal skin as this came in after 35265ba04951b5615506f6929c2c9d66e4e61c66. Though, this might be perfectly ok :)

Browser console warning that we should probably fix before going live. Bringing in new stuff using deprecated stuff always feels bad :)

This page is using the deprecated ResourceLoader module "mediawiki.skinning.content".
Your default skin ResourceLoader class should use ResourceLoaderSkinModule::class

Which is from MediaWiki\Skin\WikimediaApiPortal\Skin::setupTemplateForOutput()

Echo notifications page has a few issues with the cog/preferences menu item:

ULS has same issues as Chameleon, will reopen and change T254305: WikimediaApiPortal skin doesn't play well with ULS:

skinname-wikimediaapiportal needs defining so it displays nicely (even if it's going to be the default/only skin on the wiki)

Should the personal tools dropdown thing include a link to Special:Preferences?

The main page also seems "too wide", with no borders. Like why does everything go right upto the left and right sides?

Other pages seem fine

In T246949#6268555, @Reedy wrote:

The main page also seems "too wide", with no borders. Like why does everything go right upto the left and right sides?

Other pages seem fine

Thanks for catching this, @Reedy! I've added this to the list for design review (T256793) as well as a few other callouts from your comments.

To answer some of the questions:

Also, why $wgUseMediaWikiUIEverywhere?

We probably don't anymore, this was something that was copied over from Chameleon, i dont recall actually needing it. I will try and remove it

Main page:
Main page uses a special layout, which is edge-to-edge design. Since the layout and content of the main page are both declared in the content of the page, it uses the special layout to allow filling the wholepage (edge-to-edge) with content. This is the same reason why we need $wgUseRawHtml.
To remove the need for allowing raw HTML, content of the main page must be defined in the skin itself, which makes it not easily editable.

Thanks for catching some of the styling issues (like with notifications).

In T246949#6268038, @Reedy wrote:

Are we wanting/needing $wgRawHtml to be true?

I discussed the with @apaskulin, and we agreed to see what we could do in wikitext without this set, even if the content doesn't look exactly like the prototype. There are other ways to get the look that we need without setting this.

Also, why $wgUseMediaWikiUIEverywhere? We don't set it on any WMF wikis atm, though MobileFrontend seemingly does on the fly in some cases...

Agreed we should not set this.

In T246949#6268529, @Reedy wrote:

default.mustache has random </body></html> at the end, which don't match anything. Even if the opening ones are brought in programatically via some other part of the template, it feels a bit odd to be closing them manually... But that's maybe just how these things are done

I discussed this with @ItSpiderman, and if I understand correctly, this was introduced by one of @Krinkle's patches. @Krinkle, do you have any insight?

Bootstrap 4.3.1 is the latest version of the 4.3 branch, but 4.5.0 is the latest release of the 4.x branch.

https://github.com/twbs/release says 4.x is the active LTS, going into Maintenance LTS on 2020-05-26 and is due to be EOL before the year is out on 2020-11-26.

Bootstrap 5 is in alpha, and due for release late this year.

What is the maintenance plan for keep this relatively up to date (at least, as far as keeping on "supported" versions)?

This definitely needs to be addressed. That is one of the implications of not using Chameleon/Bootstrap so now we need to do this maintenance on the skin. From the versions you quoted, it sounds like there will need to be an update to the skin to accommodate the new Bootstrap version by the end of the calendar year.

The requirement for "composer/installers": "~1.0" should be removed, AIUI?

I guess that depends upon the outcome of T250406. If the skin supports composer download (in addition to the traditional means), this line is necessary to ensure the skin gets put in the skins directory and not the vendor directory.

Why are we loading the logo from the extension path?

We should not be.

I'm going to mark this as "done" as far as the security review is done.

I will note I'm not overly happy about the mix of $wgRawHtml and it being a SUL (even if locked down) wiki.

This isn't something we support elsewhere (ie SUL and RawHtml; other wikis are locked down), so not something we want to be encouraging. But obviously as Cindy (and Alex) have said above, they're going to try without using it, and see how far things get and see where things stand; so this as is isn't a "you shall not continue".

Also, the aformentioned Bootstrap maintenance plan. Bootstrap 4.x is now in maintenance LTS as of 2020-05-26, and stays that way till 2020-11-26. Obviously, as 5.x is still not released yet a stable release (there is a 5.x alpha as of 2020-06-16), I'm not going to recommend upgrading just yet. I will file a task for doing so (ideally before 4.x is actually EOL in November), and would appreciate it if that is scheduled into the active work program as things progress. Based on 4.x support cycle, it should be supported for 2.5-3 years, so an upgrade to 6.x presumably wouldn't be needed for a couple of years.

Thank you, @Reedy!