Page MenuHomePhabricator

REST Handler::getValidatedParams() return contract
Closed, ResolvedPublic


In an unrelated code review, it looks like there might be a code smell in Handler::getValidatedParams.

From my basic understanding, the method is allowed to return null or array, where null indicates we're early in the processing and validate() hasn't been called yet, and array is a fully-formed array of parameters.

The PageHistoryHandler::run implementation, requires its return value to be an array, but has no way to enforce this currently, nor is it responsible for handling the error. At glance, it would seem like one of two things should happen:

  1. For getValidatedParams() to enforce that it only be called after validate() and thus throw if that is the case (instead of null).
  2. Or, if null values from getValidatedParams() are needed elsewhere, for Handler::run to inject this array upfront, so that it doesn't have to pull it from getValidatedParams() and thus need to support null returns.

This is additionally problematic because if this contract were to be broken right now by accident, there is not even a run-time error. The null will silently tolerate the bracket-access on its keys and continue on, emitting warnings at every step of the way, but not stopping for anything, likely not producing a pre-determined outcome, perhaps even a successful response with unintended data.

This came up in reviewing due to the current code being flagged by Phan as "suspicious".


Related Gerrit Patches:

Event Timeline

Krinkle created this task.Oct 12 2019, 11:49 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 12 2019, 11:49 PM

Change 550991 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/core@master] REST: Make getValidatedParams() always return an array

Change 550991 merged by jenkins-bot:
[mediawiki/core@master] REST: Make getValidatedParams() always return an array

Krinkle closed this task as Resolved.Jan 8 2020, 4:49 PM
Krinkle assigned this task to tstarling.