Page MenuHomePhabricator

Math extension should follow Best practices for extensions
Open, MediumPublic

Description

The Math extension should follow the Best practices for extensions to create a pleasant atmosphere for new contributors T183318.

I @Physikerwelt, would like to make the Math extension and the associated services a place that attracts new contributors and allows for a continuous and workflow. The old complex code base currently blocks that. In the future, no obstacles should be on the path from concept to research prototype to beta feature to the production environment.

Meta

  • MUST: Use the normal MediaWiki bug tracker / code review systems
  • MUST: For deployment on Wikimedia wikis – performance & security review
  • SHOULD: The extension should not be Wikimedia or organization specific
  • SHOULD: Have co-maintainers! Doesn't matter if it's a Wikimedia Foundation employee, but there should be 2 minimum.
  • SHOULD: Create a MediaWiki-Vagrant role for your extension.

It is unmaintained. I switched to Docker.

Architecture

  • SHOULD: Provides all the same functionality over API and web (index.php)
  • RECOMMENDED: The same functionality should be implemented in a way without any duplication
  • SHOULD: Use dependency injection, avoid static calls for other than utility methods or hook entry points
  • SHOULD: Avoid global state (TODO: expand)
  • SHOULD: Be easy to debug
  • SHOULD: Use structured logging, with meaningful levels
  • SHOULD: Throw exceptions only for situations it should not handle and need to bubble up
  • SHOULD: Don't hardcode wikicode / templates or stuff, especially in a way that's not configurable for other websites
  • SHOULD: Code should be readable by someone who is familiar in that area
  • SHOULD: Don't call out to static functions in an unrelated class because it wasn't refactored or thought out well
  • SHOULD: Has a clear separation of concerns between what it actually does, and how its presented to the user
  • SHOULD: Think twice before adding new wikitext syntax functionality or something that will have a stable API for a very long time
  • SHOULD: Skin and extension functionality should not be tightly integrated.
  • SHOULD: Don't add new user preferences, unless you really have a good reason to do so
  • RECOMMENDED: Expose your JavaScript methods so 1) user scripts can access them, 2) it is easy to debug them. Do not make everything private.

Documentation

  • MUST: Have an Extension:… page on mediawiki.org
  • MUST: Add it to the appropriate extension categories
  • MUST: Quick explanation of what it does
  • MUST: Document available hooks under Extension:ExtensionName/Hooks/HookName using the "ExtensionHook" template (if any)
  • MUST: Complete list of any dependencies and how to install them, configure, and uninstall* (TODO: clarify uninstall)
  • SHOULD: Declare the compatibility policy of your extension.
  • SHOULD: All configuration settings described in one place, from most-used to most-obscure
  • RECOMMENDED: Compare and contrast with similar extensions
  • RECOMMENDED: Document hooks used in the extension infobox, it's a nice method of exposing examples so that other developers can learn (they get automagically categorised).
  • RECOMMENDED: Instructions how to/information on limitations regarding uninstallation
  • MUST: Having a Help:Extension:… page on mediawiki.org
  • RECOMMENDED: Documentation should have screenshots in multiple languages, and ideally include one in a right-to-left language
  • RECOMMENDED: Documentation should discuss some edge cases that were tested – to prove due diligence of not just testing on simple/generic articles
  • SHOULD: Consistently use <code>, <source>, <kbd>, <pre> in documentation
  • RECOMMENDED: Add/update the extension's page on WikiApiary (if needed).

File structure

Overall, the extension's file layout should be organized: consistent naming, directory structure that is logical and not messy.

  • MUST: Using the following standard directory layout:
  • includes/ or src/: Contains all (and only) PHP classes
  • i18n files for specialpage alias, magic words or namespaces located in root folder
  • SHOULD: One class per file.
  • SHOULD: Classes in MediaWiki\Extension\<ExtensionName> namespace. MediaWiki\<ExtensionName> is permissible if the extension name is unique and not something generic.
  • SHOULD: Use PSR-4 structure for classes and files (note that as of MediaWiki 1.31 you can use AutoloadNamespaces in extension.json instead of listing all classes).
  • resources/ or modules/: Contains JavaScript and CSS for ResourceLoader.
  • maintenance/ command-line maintenance scripts
  • i18n/: Contains localized messages in JSON files.
  • sql/: SQL files for database modifications (e.g. called by LoadExtensionSchemaUpdates)
  • tests/:
  • tests/phpunit/: Contains PHPUnit test cases
  • tests/parser/: Contains parser test files
  • COPYING or LICENSE: Contains a full copy of the relevant license the code is released under
  • SHALL NOT: Shall not have a giant root level directory
  • SHOULD: Not having dozens of nested directories that all only contain one or two things
  • SHOULD: Not having giant files/many separate tiny files (but follow one class per file pattern – many tiny classes may be a sign of something else wrong)
  • SHOULD: A README file that summarizes the docs and gives detailed installation direction

Database

  • MUST: If adding database tables, it should use the LoadExtensionSchemaUpdates hook to ensure update.php works.
  • MUST: Database access should always use database abstraction layer Doing so will avoid a lot of easy SQL injection attack vectors
  • SHOULD: It works well in a distributed environment (concurrency, multiple databases, clustering).
  • Uninstallation maintenance script (dropping tables, removing added columns, deleting log entries, deleting page properties). (TODO: ???)
  • SHOULD: If it needs persistence, it creates nice SQL (primary keys, indexes where needed) and uses some caching mechanism where/if necessary
  • SHOULD: Never add fields to the core tables or modify it in any way.
  • SHOULD: Create its own table for the fields with the same primary key as the core table. This makes it easier to remove a extension.

Coding conventions

Overall, follow the MediaWiki coding conventions for PHP, JavaScript, CSS, and any other languages that could be used.

  • SHOULD: Run MediaWiki-CodeSniffer for PHP coding conventions
  • SHOULD: Run Phan for PHP static analysis
  • SHOULD: Run eslint for JavaScript coding conventions and static analysis
  • SHOULD: Run stylelint for CSS coding conventions
  • SHOULD: Don't dump everything into one function (in JavaScript especially). If you're indented off the screen, you're doing it wrong
  • RECOMMENDED: Use code comments to document why you do things, not what you do. In long blocks of code, adding comments stating what each paragraph does is nice for easy parsing, but generally, comments should focus on the questions that can't be answered by just reading the code.

Testing

  • SHOULD: Have and run PHPUnit and/or QUnit tests
  • SHOULD: If there are parser functions or tags, have and run parser tests
  • RECOMMENDED: Have and run browser tests
  • RECOMMENDED: Test against right-to-left (RTL) languages! (how to verify?)
  • RECOMMENDED: Test against language converter languages! (how to verify?)

i18n & Accessibility

Overall, your extension should be fully usable and compatible with non-English and non-left-to-right languages.

  • SHALL NOT: Don't have hardcoded non-translatable strings in your code, use the proper localisation functions (wfMessage)
  • MUST: Using i18n and Translatewiki.net, and have a clear prefix to ease search of messages to translate.
  • MUST: Use HTML elements for their intended purpose. Aka use a <button> and not a <div> with a click handler for accessibility.
  • SHOULD: Id's should only be for things you can navigate to. Everything else should be a class. Even if you want to only use it once, just use a unique class.
  • SHOULD: I18n escape as close to output as possible. Document whether functions take/except wikitext vs. HTML.
  • RECOMMENDED: Add qqq message strings for all messages that exist in en.json, and verify them using grunt-banana-checker

Security

  • SHALL NOT: Don't touch HTML after it has been sanitized (common pattern is to use regex, but that's bad)
  • MUST: Shelling out should escape arguments
  • MUST: All write actions must be protected against cross-site request forgery (CSRF)
  • SHOULD: Use the normal MediaWiki CSRF token system
  • SHOULD: Don't load external resources for privacy and performance
  • MUST: Make sure privacy related issues (checkuser, oversight) are still covered when refactoring or writing new code

Don't reinvent / abuse MediaWiki

Overall, don't re-implement functionality or code that MediaWiki provides.

  • MUST: Use MediaWiki functionality/wrappers for things like WebRequest vs. $_GET, etc.
  • MUST: Use hooks as opposed to hacks/reimplementing things.
  • MUST Use MediaWiki's validation/sanitization methods e.g. those in the Html and Sanitizer classes.
  • MUST: Don't disable parser cache unless you have a really good reason.
  • MUST: Use Composer for 3rd party PHP library management.
  • SHOULD: Don't reimplement libraries that exist, don't reinvent the wheel.
  • SHOULD: Don't disable OutputPage.
  • SHOULD: If an abstraction exists (e.g. ContentHandler), use that instead of hooks.
  • SHOULD: Don't make things harder for yourself – use standard functionality like extension.json's tests/PHPUnit auto-discovery stuff.
  • SHOULD: Use global MediaWiki configuration such as read-only mode.

Uncategorized

  • Can I read the code with reasonable effort? Is it convoluted / unnecessarily complex?
  • Know when you should use ParserOutput methods vs. same methods in OutputPage
  • It's okay to file duplicate bugs (it's better than not filing it at all!)
  • Should extensions be adding user groups in the default extension?
  • Adding user rights are easy in code, but can be politically controversial and should be discussed with the community about who grants the rights and who has them by default

Event Timeline

Physikerwelt added a project: Epic.

@mobrovac @thiemowmde @WMDE-Fisch I checked all boxes where I believe this is certainly implemented. It is now a fairly large list of unchecked points. Do you have any suggestions on how to proceed and prioritize the tasks? For instance, are there some aspects that should be tackled first?

For instance, are there some aspects that should be tackled first?

Just from quickly looking over it:

  • I think all the MUST points seem quite reasonable to me as next steps. Especially in the section of Don't reinvent / abuse MediaWiki. I would probably start there.
  • I also really like the first four steps still open in File structure, they should be easy to implement ( with a good IDE )

MUST: Use HTML elements for their intended purpose. Aka use a <button> and not a <div> with a click handler for accessibility.

You could add there to use OOUI for your UI elements. OOUI will already take care of a lot of these things and help increase accessibility

@WMDE-Fisch thank you.

MUST: Use MediaWiki functionality/wrappers for things like WebRequest vs. $_GET, etc.
MUST: Use hooks as opposed to hacks/reimplementing things.
MUST Use MediaWiki's validation/sanitization methods e.g. those in the Html and Sanitizer classes.

I think the reinvent points are really challanging to me. I have insuffcient knowledge about MediaWiki core and it's recent changes to be sure that those points are implemented. However, all the code that passed code review was checked by people with sufficient knloedge I guess. Thus I will just mark those points as done.

The file structure things are apprently done as well.

And we also DO use OOUI. So another check.

We indeed do not NOT use HTML elements for their intended purposes. In particular we intentionelly abuse image elements to display formulae, but otherwise all chrome users would not see reasonable output. I think, here we might want to update the guide itself.

So overall I get the impression that we are doing quite well.

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)