Wikimedia Bugzilla is closed!

This would need to be done differently from how FlaggedRevisions currently is implemented (as in FR you still submit a new version which is directly being used by eg. ResourceLoader).

I would suggest to have a new extension in which edits can basically be suggested like github pull requests and then get merged/ commented on/ rejected by a reviewer.

Mid to long term this process should be centralized and managing JS/CSS should move away from the Wikis into a central place (that can be a MediaWiki, but IMO doesn't necessarily have to be one).

Would it make sense to use git / phabricator ?   We will have to convince the community that a pre commit review workflow is a good thing, but that would let us run validation tests and deploy on some staging area before rolling changes to production.

I am very skeptical at our ability to code and maintain a review system in MediaWiki, we have past experiences:

- CodeReview extension for svn could have been nice if we had the ability to devote time to it, it turned out to be simpler to delegate the software to third party (Gerrit, soon Phabricator). 

- FlaggedRevs which would let one validate revisions before they were made public. I don't think anyone regret it. I found the workflow / UI etc terrible and never understood how it worked.


Also, we could use such a review system for Gadgets and LUA modules.  When writing, versionning and collaborating on code, ones need a version control system. git comes to mind.

Why can't we just do this the same way as our current status quo (gerrit) or the soon-to-be status quo (phabricator)? Less systems that do the same thing to maintain...

Some volunteers have +2 on our projects, this is the same situation except that we need to find more volunteer reviewers than usual, because we don't have the bandwidth to review this at the moment.

IMHO if the code review process we normally undergo feels heavy to impose on this audience, it just needs that we need to improve the process. If it becomes easier to outsiders, it benefits all projects.

I think the community would likely reject attempts to use git or gerrit.  Whatever this system is, it will need to be implemented within Mediawiki.  People have a hard enough time leaving their home wiki, I seriously doubt that most sysops are going to download and learn to use git.

Though Flaggedrevs is weird, I personally think that it, or something like it, is the way to go.  The learning curve of it is less than that of git and gerrit, and I'm sure some sort of bridge could be made that would allow running of tests on code submitted there.

I feel like any system outside of Mediawiki would stifle each communities autonomy, something that in recent weeks we've seen they care an awful lot about.

Indeed. This definitely must be implemented "inside" MediaWiki (whatever the backend we use, the UI must be displayed within regular MediaWiki pages) if it is supposed to be used by MediaWiki users and not MediaWiki developers.

I'm not sure this problem is being approached in the best way. I think it would be helpful to look at use-cases and work backward from there. Why are administrators adding custom site-wide JavaScript?

Looking through [[MediaWiki:Common.js]], for example, it seems as though a lot of code is deprecated and could be removed once de-referenced. The non-deprecated code should probably exist in MediaWiki core or in a MediaWiki extension, if it's generally useful functionality.

Code is the enemy; less code is better. Rather than looking at ways to impose code review on wiki communities, we should first look for ways to centralize code (global gadgets) and we should look for ways to make the current site-wide JavaScript hacks no longer necessary.

(In reply to Zell Faze from comment #4)
> I think the community would likely reject attempts to use git or gerrit. 
> Whatever this system is, it will need to be implemented within Mediawiki. 
> People have a hard enough time leaving their home wiki, I seriously doubt
> that most sysops are going to download and learn to use git.

A smart front-end could mask that the user is using Git. I'm thinking about an interface similar to in-browser editing in the GitHub front-end. But again, I'd like us to more closely look at the underlying use-cases first.

The fact that people who aren't developers and/or who don't understand what custom JS they're deploying to millions of users is the problem. It's not a situation that needs to be perpetuated through dumbed-down on-wiki code review. Like MZ just described, if there's a way to remove the bulk of the need for these users to deploy custom JS, it should be explored first.

And for whatever JS is left that can't be standardized into a global gadget repository (I haven't looked at the code, but I imagine there will always be "that specific thing that can't be done differently"), community members with appropriate skills should be doing it. If you're not willing to learn git, you're not the right person to be pushing custom JS to a top-5 website. Even if you don't want to learn or don't have time to, it just means that you need to enlist the help of someone who knows about this stuff. Right now that step doesn't exist and irresponsible actions are being taken by people who are just unaware of what they're doing.

I think that a "simple mode" review tool is also likely to make the uneducated people sending JS they don't understand less likely to learn to master what they're doing. As such it wouldn't be code review, it would be "please make this thing I don't understand work and deploy it", which puts all the workload burden on the reviewer, instead of being the 2-way street code review should be.

I think that extensions and gadgets are probably the "right" way to make sitewide changes.  Extensions already have a code review/deployment process, although we can certainly work to make that smoother/easier.  Perhaps enwiki could have an 'Extension:enwiki', for example, which could contain any code which would otherwise live in common.js.  On github, at least, proposing a new change would be as easy as going to the 'common.js' file inside the Extension:enwiki respository and clicking 'edit', which takes care of forking, branching, commiting, and submitting a pull-request.

Gadgets don't really have a good review mechanism at the moment.  I think this bug could concentrate on reviews for gadgets;  common.js is just a weird special case that doesn't need to exist.

(In reply to Gilles Dubuc from comment #7)
> The fact that people who aren't developers and/or who don't understand what
> custom JS they're deploying to millions of users is the problem. It's not a
> situation that needs to be perpetuated through dumbed-down on-wiki code
> review.

I've heard that same argument as to why wikitext is better than VisualEditor. I didn't agree then, and I don't agree now: making the process technically difficult to try to discourage "unqualified" contributors isn't a very well-targeted heuristic. Even if the people affected are smart enough to be able to figure out the process, they may well decide the effort isn't worth it.


(In reply to MZMcBride from comment #6)
> Code is the enemy; less code is better. Rather than looking at ways to
> impose code review on wiki communities, we should first look for ways to
> centralize code (global gadgets) and we should look for ways to make the
> current site-wide JavaScript hacks no longer necessary.

While having more code in gadgets rather than in common.js is good, it's orthogonal to the need for a good code review process that's actually usable by wiki users. The global gadgets will still need a code review process.

Wiki users might accept having to go to a Commons-like site instead of doing it on their local wiki (and in the long run they'll probably have to), but they're unlikely to accept something that requires they install software locally, sign up for a non-SUL account, figure out ssh keys, publish an email address publicly, and so on.

FlaggedRevs isn't a good model, because we need merging of proposed changes rather than a linear history. I don't know whether CodeReview will do pre-commit review, which is probably a necessity.

Notice that many wikis have migrated code from Common.js to default gadgets (e.g. for modularization, to allow users to disable functionality, to make debugging easier, to allow importing specific parts from other wikis, etc), and others also load subpages like Common.js/edit.js, Common.js/watchlist.js, Common.js/IEFixes.js, etc..

Wikimedia Commons for example has 17 gadgets enabled for everyone by default. See more stats on
https://meta.wikimedia.org/wiki/Gadgets

(In reply to Brad Jorsch from comment #9)
> ...
> they're unlikely to accept something that requires they install software
> locally, sign up for a non-SUL account, figure out ssh keys, publish an
> ...
FYI: we might have SUL for Phabricator:
http://fab.wmflabs.org/T314

I don't think MediaWiki should invent its own code review software. I completely agree with every Gilles Dubuc and C. Scott said above.

Code review software is /hard/ to build and for us to build it would 1) be a waste of resources and 2) be suboptimal.


That aside, The existing MediaWiki:Common.js is a serious hack and huge security hole that I can't believe has not been exploited yet and we really should be thinking about better ways to replace it with something else.

I too think Gadgets and extensions are the place for these things. Anything site wide should be in an extension, anything goes in a gadget as long as user has the choice to opt into it.

MZMcBride is spot on and makes a good suggestion that we should be starting at the source.
To take English wikipedia as an example [1]
When I look through the code I see:

* Main page layout fixes - why is this code running across the entire site?
* A redirect script that sends User:Name/skin.js to User:Name/skin.js to vector.js or monobook.js - again why is this running across the entire site and not a feature of the software?
* Some deprecation notices... shouldn't deprecation happen in the software?
* Then a bunch of imported scripts:
** if you are on an edit page 'MediaWiki:Common.js/edit.js'
** on the watchlist MediaWiki:Common.js/watchlist.js
** on the file page MediaWiki:Common.js/file.js

At this point I gave up reading. All these indicate to me is that there are holes in the software that need fixing for all users and are being neglected to be fixed in the software because "code review is too cumbersome". This is completely irresponsible and upsets me. We should be striving to make MediaWiki as customisable and flexible as possible without having hacky compromises like this.

"sysops are going to download and learn to use git." if a person is not able to learn git then personally I worry about them tinkering with software that hits site wide. Tinkering with gadgets that are available for opt in is a great thing and I believe important for community innovation but site wide just scares me.

My suggested way forward would be:
* Audit all uses of Common.js
* Open bugs for all the problems they are solving (I think we'll be surprised at how much overlap there is between projects)
* Work with the community to fix the issues in the software they have identified in Common.js and remove the code from Common.js. In doing so build up a trust that through code review we can fix things just as quickly and better without resorting to needing a wikipage.
* With a cleaner Common.js wikipage and better process see the wood through the trees and reevaluate what we need this page for.

[1] https://en.wikipedia.org/wiki/MediaWiki:Common.js

I think code review isn't the solution, as it is probable that no one will review the code for small wikis. 
However it would be great to have automated tests running after changes in common.js and gadgets. Such general tests can at least validate no external resource is loaded, or no invalid syntax is added.
The simplest option is to run it outside MW (such as a script in labs that runs X times a day), but an extension to do it while editing would be even better...

BTW
I just runned a simple phantomjs script on all wikipedia languages to catch simple errors:

testing als
ERROR: ReferenceError: Can't find variable: ta
TRACE:
 -> https://als.wikipedia.org/w/index.php?title=MediaWiki:If-pt-login.js&action=raw&ctype=text/javascript&dontcountme=s: 9

testing kk
ERROR: TypeError: 'null' is not an object (evaluating 'document.getElementById('mw-vector-current-variant').innerHTML=wgULS("Кирил","Latın","توتە")')
TRACE:
 -> https://bits.wikimedia.org/kk.wikipedia.org/load.php?debug=false&lang=kk&modules=site&only=scripts&skin=vector&*: 9


anyone wants to fix them? :)

Created attachment 16185 [details]
Example for errors across wikis

Maybe it is a bit off-topic but here are some cases of errors across wikis in simple read/edit transaction using phantomJS

Eran Roz: please fill bug report for all such issues. This way they can be triaged / assigned to the proper persons and will be easier to track than this bug which is merely for discussion.

I think that the problem is that the MediaWiki namespace can be edited by any administrator. Many administrators do not understand what the code does, as demonstrated for example by the "~~~~" in [[szl:Special:PermanentLink/207947]], which Legoktm mentioned above. A simple solution would be to remove the right to edit pages in the MediaWiki namespace from administrators and instead assign it to a new group, "scripteditors", consisting of editors who understand how the code works.

I think that some code should be removed from Common.js files and moved to MediaWiki core instead. For example, [[sv:MediaWiki:Common.js]] contains code fixing incorrect sorting of text in tables (search for "tableSorterCollation"). Every user of MediaWiki would want text to be sorted correctly, so I don't see why this couldn't be in MediaWiki core instead.

Gadgets 2.0 introduces new user rights specific for edit/management of gadgets:
https://www.mediawiki.org/wiki/ResourceLoader/Version_2_Design_Specification#User_rights
See also this discussion about which users (user groups) should have each permission:
https://www.mediawiki.org/wiki/Thread:Talk:ResourceLoader/V2_testing/Questions_about_permission_model_and_developer_workflow

(In reply to Eran Roz from comment #13 and comment #14)
> I just runned a simple phantomjs script on all wikipedia languages to catch
> simple errors:
> 
> [...]

Thanks for this. I split this out to bug 69519.

(In reply to Stefan2 from comment #16)
> I think that the problem is that the MediaWiki namespace can be edited by
> any administrator. Many administrators do not understand what the code does,
> as demonstrated for example by the "~~~~" in
> [[szl:Special:PermanentLink/207947]], which Legoktm mentioned above. A
> simple solution would be to remove the right to edit pages in the MediaWiki
> namespace from administrators and instead assign it to a new group,
> "scripteditors", consisting of editors who understand how the code works.

Err, most of the MediaWiki namespace isn't JavaScript and CSS, it's interface messages. The fact that JS and CSS pages are included in the MediaWiki namespace has always been a design quirk that should probably be corrected as their inclusion in the namespace is a(n) (ab|mis)use of the namespace's purpose.

Broadly, administrators are trusted users and should be treated as such.

Whether the rest of the MediaWiki namespace is needed is also debatable nowadays, but probably also outside the scope of this ticket.

> I think that some code should be removed from Common.js files and moved to
> MediaWiki core instead. For example, [[sv:MediaWiki:Common.js]] contains
> code fixing incorrect sorting of text in tables (search for
> "tableSorterCollation"). Every user of MediaWiki would want text to be
> sorted correctly, so I don't see why this couldn't be in MediaWiki core
> instead.

Yes, for sure. I made a similar comment above (comment 6). We should figure out why these pages are still needed and work to address that by putting the functionality in MediaWiki core or in an extension. I think this is a better use of time than proposing trashing the functionality.

In other words, if you all want to deprecate the use of MediaWiki:Common.js and similar pages, you should work toward that goal. If/when the day comes that they're no longer needed, then we can consider getting rid of them. But this bug seems to be a bit "cart before the horse."

(In reply to Gilles Dubuc from comment #7)
> The fact that people who aren't developers and/or who don't understand what
> custom JS they're deploying to millions of users is the problem.

You're basically hitting [[hard cases make bad law]] here. Most Wikimedia wikis (probably over 800 of them) do not have millions of users.

> If you're not willing to learn git, you're not the right person to be
> pushing custom JS to a top-5 website.

Can we please collectively stop with the "top-5" bullshit? :-)  Obviously the English Wikipedia and other wikis are a consideration, but over 99% of MediaWiki wikis are not top-5 anything.

(In reply to Brad Jorsch from comment #9)
> (In reply to MZMcBride from comment #6)
>> Code is the enemy; less code is better. Rather than looking at ways to
>> impose code review on wiki communities, we should first look for ways to
>> centralize code (global gadgets) and we should look for ways to make the
>> current site-wide JavaScript hacks no longer necessary.
> 
> While having more code in gadgets rather than in common.js is good, it's
> orthogonal to the need for a good code review process that's actually usable
> by wiki users. The global gadgets will still need a code review process.

It would be helpful if you could explicitly specify what needs you see that you feel MediaWiki could handle well and/or that you think Phabricator could not handle well.

> Wiki users might accept having to go to a Commons-like site instead of doing
> it on their local wiki (and in the long run they'll probably have to), but
> they're unlikely to accept something that requires they install software
> locally, sign up for a non-SUL account, figure out ssh keys, publish an
> email address publicly, and so on.

GitHub and GitLab both allow in-browser editing now, I believe. I imagine Phabricator, which will allegedly be part of Wikimedia unified login, will also have equivalent functionality. The e-mail address is trivially solvable in Git by simply making up an e-mail address (User:Anomie@git-en.wikipedia.org), I think? And if you're using a Web browser and single unified login for authentication, you probably don't need to worry about SSH keys or installing software such as Git locally.

You may be correct that there's a need here for global gadgets, but I think we need to do more to be much clearer about what that need is and how we propose to address it, assuming additional development—outside of the ongoing Phabricator work and I suppose the work to deprecate Common.js and similar pages—is needed.

(In reply to C. Scott Ananian from comment #8)
> I think that extensions and gadgets are probably the "right" way to make
> sitewide changes.  Extensions already have a code review/deployment process,
> although we can certainly work to make that smoother/easier.  Perhaps enwiki
> could have an 'Extension:enwiki', for example, which could contain any code
> which would otherwise live in common.js.

I was thinking a bit more generic, but yeah, this might be a good idea to pursue. My thought was that you might have Extension:WikimediaScripts, which follows the pattern of E:WikimediaMaintenance and E:WikimediaMessages and E:WikimediaEvents. Inside the WikimediaScripts extension, you'd have modules that could then be loaded on a per-wiki basis.

> common.js is just a weird special case that doesn't need to exist.

I addressed this line of reasoning in comment 19, last paragraph. For now, there's a definite need for these pages to exist. That may not be true in the future, but it is true today.

(In reply to MZMcBride from comment #19)
> Err, most of the MediaWiki namespace isn't JavaScript and CSS, it's
> interface messages. The fact that JS and CSS pages are included in the
> MediaWiki namespace has always been a design quirk that should probably be
> corrected as their inclusion in the namespace is a(n) (ab|mis)use of the
> namespace's purpose.

Well yes, but there are some messages that are also a big issue. See also bug 43646

(In reply to MZMcBride from comment #19)
> In other words, if you all want to deprecate the use of MediaWiki:Common.js
> and similar pages, you should work toward that goal. If/when the day comes
> that they're no longer needed, then we can consider getting rid of them. But
> this bug seems to be a bit "cart before the horse."

Agreed, and if anyone wants to write patches that make our software more useful I will happily help merge them. Have raised bug 69550 so as not to derail this bug anymore. :)

(In reply to Stefan2 from comment #16)
> I think that the problem is that the MediaWiki namespace can be edited by
> any administrator. Many administrators do not understand what the code does,
> as demonstrated for example by the "~~~~" in
> [[szl:Special:PermanentLink/207947]], which Legoktm mentioned above. A
> simple solution would be to remove the right to edit pages in the MediaWiki
> namespace from administrators and instead assign it to a new group,
> "scripteditors", consisting of editors who understand how the code works.
> 

> Broadly, administrators are trusted users and should be treated as such.

Yes but whereas I might trust a fellow developer, I still wouldn't at all me comfortable if they +2ed their own code and this is why we have code review processes. Also I might trust an administrator in wiki things but if they are unable to use a code review tool then I wouldn't trust them to edit JavaScript. I worry that a lot of this is due to an outdated idea that JavaScript is not a real language. Ask yourself what if MediaWiki:Common.php existed and how allowing editing of code on a wiki that would make you feel...

I can imagine Common.js being useful for a hobby wiki to make a few tweaks, but it really has no place on a Wikimedia wiki and we should certainly not be building code review systems to support it. Instead we should be focusing energy on making code review in mediawiki core quicker and more enticing and more regularly deploying our code.

Depends on bug 20153?

Why not do it as page correction and validation in Wikisource? It is a light process, and there is no need to learn a new tool.

With the Proofreading page extension, 2 non-anonymous users are needed to validate a page. Additionally, more right could be needed (either sysop or autopatrolled).

(In reply to Yann Forget from comment #27)
> Why not do it as page correction and validation in Wikisource? It is a light
> process, and there is no need to learn a new tool.
> 
> With the Proofreading page extension, 2 non-anonymous users are needed to
> validate a page. Additionally, more right could be needed (either sysop or
> autopatrolled).

This sounds good to me. +1

What about creating the "Gadget developer" user group on WMF wikis, with the user rights which are currently given to sysops (edituserjs, editusercss and editinterface - which Gerrit change #154452 splits into 'editsitejs' and 'editsitecss') and, later, also the user rights introduced by Gadgets 2.0 (see the links mentioned on comment 17).

Then, send an announcement recommending communities to evaluate whose of their sysops should indeed have the permissions to edit JavaScript and CSS and add them to the new user group, and after a reasonable amount of time, remove the user rights from sysops on WMF wikis.

Also, since it is recommended to keep gadgets central[1], and since now Meta-wiki is the central wiki used by extension GlobalCssJs, maybe apply any code-review process first to the editing of global gadgets hosted on Meta (some are currently hosted on mw.org or enwiki, or wikidata, but using a single common wiki would be the best - whatever wiki is choosen - and would help in the implementation of the Gadgets 2.0 when that is ready for deployment in the future).

[1] https://www.mediawiki.org/wiki/ResourceLoader/Migration_guide_%28users%29#Keep_gadgets_central

(In reply to Helder from comment #29)
> using a single common wiki would be the best - whatever wiki is choosen -

mediawiki.org please. I'm happy to discuss the reasoning wherever corresponds.

(In reply to Quim Gil from comment #30)
> mediawiki.org please. I'm happy to discuss the reasoning wherever
> corresponds.
Ideally both global user scripts and global gadgets should be in the same wiki (but Meta is already in use for the first, so going to another wiki will require migration - which is easy enough: 1. inform users; 2. import any /global.js/css from Meta, with history; 3. delete the old ones from Meta).

Anyway, both Meta and mw.org have Translation extension installed, which will help creating multilingual documentation in a consistent way for popular gadgets.

I think the effort of bringing all developers under the same roof is worth, and mediawiki.org is the right roof. 

But going back to the definition of the sane code-review process, some ideas came up from a casual email chat. Summary in sequential order:

Erik proposed to investigate the idea of Phabricator with a web based interface for editing files, as a possibility to bring easy-as-in-editing-text development closer to proper code review and continuous integration with automated QA.

We checked upstream, but generally speaking they are not enthusiastic with the idea. See their reasoning at https://secure.phabricator.com/T6008#7

For different reasons, Legoktm proposed to go back to the idea of a MediaWiki-based code review. Let me paste:

"(...) we could just put a revision id in the gadget definition:

"scripts": {"foo.js": 123, "bar.js": 456},
"styles": {"baz.css": 789},

Or something like that. So anyone can edit (maybe still restricted by
a userright or maybe not) the JS/CSS pages, but their changes don't
actually take effect until the gadget definition is updated by someone
with those rights (+2). This allows us to take advantage of wiki
process like be bold, revert, discuss, but still impose a review
requirement by someone with "+2" rights. In some ways it's like SVN."

As Legoktm explain, there is a precedent of this type of process in the workflow for approving EventLogging schemas, where many can edit the schemas but only a few can point to a new version (the +2 equivalent).

https://www.mediawiki.org/wiki/Extension:EventLogging/Guide#Peer_review