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:
- For getValidatedParams() to enforce that it only be called after validate() and thus throw if that is the case (instead of null).
- 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 https://gerrit.wikimedia.org/r/542672 due to the current code being flagged by Phan as "suspicious".