Don't expect objects by reference in hook handlers

The motivation for this patch is to make the code less complex, better
readable, and less brittle.

Example:

public function onExampleHook( Parser &$parser, array &$result ) {
    /* This is the hook handler */
}

In this example the $result array is meant to be manipulated by the
hook handler. Changes should become visible to the caller. Since PHP
passes arrays by value, the & is needed to make this possible.

But the & is misplaced in pretty much all cases where the parameter is
an object. The only reason we still see these & in many hook handlers
is historical: PHP 4 passed objects by value, which potentially caused
expensive cloning. This was prevented with the &.

Since PHP 5 objects are passed by reference. However, this did not
made the & entirely meaningless. Keeping the & means callees are
allowed to replace passed objects with new ones. The & makes it look
like a function might intentionally replace a passed object, which is
unintended and actually scary in cases like the Parser. Luckily all
Hooks::run I have seen so far ignore unintended out-values. So even if
a hook handler tries to do something bad like replacing the Parser
with a different one, this would not have an effect.

Removing the & does not remove the possibility to manipulate the
object. Changes done to public properties are still visible to the
caller.

Unfortunately these & cannot be removed from the callers as long as
there is a single callee expecting a reference. This patch reduces the
number of such problematic callees.

Change-Id: I21d53c989ea487607dc69e6b3365c023ef6729f5
This commit is contained in:
Thiemo Kreuz 2018-05-15 17:50:08 +02:00
parent 7eed63d895
commit 670f0bb8f9
4 changed files with 7 additions and 7 deletions

View file

@ -372,11 +372,11 @@ class MathHooks {
}
/**
* @param Parser &$parser
* @param Parser $parser
* @param string &$text
* @return bool
*/
public static function onParserAfterTidy( &$parser, &$text ) {
public static function onParserAfterTidy( $parser, &$text ) {
global $wgMathoidCli;
if ( $wgMathoidCli ) {
MathMathMLCli::batchEvaluate( self::$tags );

View file

@ -49,7 +49,7 @@ class MathMathML extends MathRenderer {
}
}
public static function batchEvaluate( &$tags ) {
public static function batchEvaluate( array $tags ) {
$rbis = [];
foreach ( $tags as $key => $tag ) {
/** @var MathRenderer $renderer */

View file

@ -12,11 +12,11 @@ use \MediaWiki\MediaWikiServices;
class MathMathMLCli extends MathMathML {
/**
* @param array &$tags math tags
* @param array[] $tags math tags
* @return bool
* @throws MWException
*/
public static function batchEvaluate( &$tags ) {
public static function batchEvaluate( array $tags ) {
$req = [];
foreach ( $tags as $key => $tag ) {
/** @var MathMathMLCli $renderer */

View file

@ -13,7 +13,7 @@ class MathWikidataHook {
/**
* Add Datatype "Math" to the Wikibase Repository
* @param array &$dataTypeDefinitions
* @param array[] &$dataTypeDefinitions
*/
public static function onWikibaseRepoDataTypes( array &$dataTypeDefinitions ) {
global $wgMathEnableWikibaseDataType;
@ -60,7 +60,7 @@ class MathWikidataHook {
/**
* Add Datatype "Math" to the Wikibase Client
* @param array &$dataTypeDefinitions
* @param array[] &$dataTypeDefinitions
*/
public static function onWikibaseClientDataTypes( array &$dataTypeDefinitions ) {
global $wgMathEnableWikibaseDataType;