Page MenuHomePhabricator

Introduce library for assertions for parameter validation and post-condition checking
Closed, ResolvedPublic

Description

This is a proposal for providing an alternative to PHP's ''assert()'' that allows for an simple and reliable way to check preconditions and postconditions in MediaWiki code.

The background of this proposal is the reoccurring discussions about whether PHP's ''assert()'' can and should be used in MediaWiki code. Two relevant threads:

The outcome appears to be that

  • assertions are generally a good way to improve code quality
  • but PHP's ''assert()'' is broken by design

Following a suggestion by Tim Starling, I propose to create our own functions for assertions. A first implementation is available at https://github.com/wmde/Assert and can be installed using composer.

Usage

function frob( $foo ) {
    Assert::parameterType( 'string', $foo, 'foo' );
    Assert::parameter( $foo !== '', 'foo', 'must not be empty' );

    //...

    Assert::postcondition( is_string( $result ), 'string expected' );
    return $result;
}

This originated as https://www.mediawiki.org/wiki/Requests_for_comment/Assert. Code is at https://github.com/wmde/Assert

Event Timeline

daniel created this task.Feb 27 2015, 3:35 PM
daniel raised the priority of this task from to Needs Triage.
daniel updated the task description. (Show Details)
daniel added a project: TechCom-RFC.
daniel moved this task to Under discussion on the TechCom-RFC board.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 27 2015, 3:35 PM
daniel updated the task description. (Show Details)Feb 27 2015, 3:35 PM
daniel set Security to None.
Spage updated the task description. (Show Details)Mar 25 2015, 6:55 PM

While our usage of it is fairly far out, especially in terms of us using php 7, we should at least consider the implications with the new php 7 expectations feature which replaces the current assertions and deals with many of the concerns raised in the above threads: https://wiki.php.net/rfc/expectations

(I'm unclear whether to discuss here or on the RFC's talk page)

What's wrong with existing libraries?

matmarex updated the task description. (Show Details)Apr 8 2015, 9:35 PM
Spage assigned this task to daniel.Apr 9 2015, 2:14 AM
Spage added a subscriber: daniel.

Approved by Tim Starling in 2015-04-08 IRC RFC discussion. From meeting minutes, the action items for @daniel:

  • change @license from GPL+ to MIT
  • add a note about when and where to use assertions
  • add exception base class
  • add a common base class for all assertion related exceptions
Spage triaged this task as Normal priority.Apr 9 2015, 2:15 AM
Spage updated the task description. (Show Details)
Spage moved this task from Under discussion to Approved on the TechCom-RFC board.

The action items from the RFC discussion have been addressed in the 0.2 release, see https://github.com/wmde/Assert/tree/v0.2.0

Remaining task: add this to MediaWiki's composer dependencies, and use it in a few places in core, to showcase usage.

For the record, in reply to @Spage's question about existing libraries: yes, there is quite a few of them, but I haven't found one that I liked. In particular, it want it to be simple (unlike Symphony's), and tailored to our use case (unlike PHPUnit). From the ones you propose, the one that seems most similar to what I envision is the Phix contract library, but looking at the code, even that looks overly complicated.

Change 205884 had a related patch set uploaded (by Daniel Kinzler):
Start using the Assert helper class for checking parameters.

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

Change 206117 had a related patch set uploaded (by Daniel Kinzler):
Adding dependency on wikimedia/assert module

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

Change 206117 merged by jenkins-bot:
Adding dependency on wikimedia/assert module

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

Change 205884 merged by jenkins-bot:
Start using the Assert helper class for checking parameters.

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

daniel closed this task as Resolved.May 11 2015, 8:10 PM

at long last!

This broke my vagrant test wiki. "Class undefined: Wikimedia\Assert\Assert "

@Cenarium: composer update should fix it. If we use composer to manage dependencies, running composer update is a must whenever the code is updated. Or did running composer update not fix this for you?

bd808 added a subscriber: bd808.May 13 2015, 3:15 PM

This broke my vagrant test wiki. "Class undefined: Wikimedia\Assert\Assert "

Folks are apparently still getting used to the new-ish world of Composer managed libraries in use by MediaWiki core. For the MediaWiki-Vagrant users out there, vagrant git-update will update your MediaWiki core and extension clones, ensure that composer update is run and finally run update.php to keep your database in sync. For other users, you can either clone mediawiki/vendor.git and update it at the same time you update mediawiki/core.git or use something like this git hook to run Composer whenever composer.json changes:

1#!/usr/bin/env bash
2# Run composer if composer.json has changed
3
4git diff-tree -r --name-only --no-commit-id ORIG_HEAD HEAD |
5grep --quiet composer.json &&
6composer update --no-progress --optimize-autoloader

@bd808: thanks for the details! I suppose these instructions should be on Manual:Upgrading... that page doesn't mention Composer nor Vagrant at the moment.

bd808 added a comment.May 13 2015, 3:44 PM

@bd808: thanks for the details! I suppose these instructions should be on Manual:Upgrading... that page doesn't mention Composer nor Vagrant at the moment.

There are links there to https://www.mediawiki.org/wiki/Download_from_Git#Fetch_external_libraries which does describe the manual Composer and mediawiki/vendor.git options, but the lead is definitely buried. I took a shot at making this a bit more prominent: https://www.mediawiki.org/w/index.php?title=Manual%3AUpgrading&type=revision&diff=1649507&oldid=1633079

composer update fixed the issue, thanks.

Krinkle moved this task from Approved to Implemented on the TechCom-RFC board.Feb 10 2016, 9:33 PM
Krinkle edited projects, added TechCom-RFC (TechCom-Approved); removed TechCom-RFC.
Krinkle moved this task from Untriaged to Implemented on the TechCom-RFC (TechCom-Approved) board.
Krinkle edited projects, added Librarization; removed Proposal, Patch-For-Review.