Page MenuHomePhabricator

Hook parameters should not be passed by reference unless the parameter is documented to be replaceable
Closed, ResolvedPublic

Description

Several hooks (like e.g. PageContentSaveComplete) are currently called with parameters passed by reference, even though this is not necessary, not required per the specification of the hook, and actually dangerous: in many cases, such parameters cannot safely be replaced by a hook handler.

This behavior is probably a relict from PHP 3 times, when objects would be copied if not passed by references.

NOTE: this cannot be changed without first changing the signature of all hook handlers. Failing to do so will result in warnings like Warning: Parameter 1 to AbuseFilterHooks::onPageContentSaveComplete() expected to be a reference, value given.

Details

SubjectRepoBranchLines +/-
mediawiki/extensions/BlueSpiceFoundationmaster+5 -5
mediawiki/extensions/SpellingDictionarymaster+1 -1
mediawiki/extensions/examplesmaster+1 -1
translatewikimaster+1 -1
mediawiki/coremaster+170 -165
mediawiki/extensions/ConfirmEditmaster+24 -19
mediawiki/coremaster+42 -42
mediawiki/extensions/GrowthExperimentsmaster+15 -15
mediawiki/extensions/Echomaster+6 -6
mediawiki/extensions/ProofreadPagemaster+11 -11
mediawiki/extensions/Quizmaster+8 -8
mediawiki/extensions/QuizTabulatemaster+2 -2
mediawiki/coremaster+16 -16
mediawiki/coremaster+10 -13
mediawiki/extensions/XAnalyticsmaster+4 -4
mediawiki/extensions/TocTreemaster+2 -2
mediawiki/extensions/TheWikipediaLibrarymaster+5 -5
mediawiki/extensions/Translatemaster+5 -5
mediawiki/extensions/TorBlockmaster+10 -5
mediawiki/extensions/TimedMediaHandlermaster+9 -9
mediawiki/extensions/FlaggedRevsmaster+7 -7
mediawiki/extensions/PageAssessmentsmaster+15 -10
mediawiki/extensions/WikimediaEditorTasksmaster+8 -8
mediawiki/extensions/TitleBlacklistmaster+2 -2
mediawiki/extensions/WikimediaEventsmaster+9 -9
mediawiki/extensions/TemplateStylesmaster+4 -4
mediawiki/extensions/timelinemaster+2 -2
mediawiki/extensions/Scribuntomaster+10 -10
mediawiki/extensions/wikihieromaster+2 -2
mediawiki/extensions/SyntaxHighlight_GeSHimaster+4 -4
mediawiki/extensions/SubPageList3master+2 -2
mediawiki/extensions/SiteMatrixmaster+2 -2
mediawiki/extensions/MapSourcesmaster+7 -7
mediawiki/extensions/RelatedArticlesmaster+7 -6
mediawiki/extensions/LiquidThreadsmaster+16 -14
mediawiki/extensions/GeoDatamaster+7 -7
mediawiki/extensions/Josamaster+2 -2
mediawiki/extensions/intersectionmaster+2 -2
mediawiki/extensions/Insidermaster+4 -4
mediawiki/extensions/InputBoxmaster+2 -2
mediawiki/extensions/ImageMapmaster+2 -2
mediawiki/extensions/GeoCrumbsmaster+9 -9
mediawiki/extensions/FundraiserLandingPagemaster+2 -2
mediawiki/extensions/WikimediaIncubatormaster+8 -8
mediawiki/extensions/Scoremaster+1 -1
mediawiki/extensions/RandomAreamaster+1 -1
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 551030 merged by jenkins-bot:
[mediawiki/extensions/Score@master] Stop passing objects by reference

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

Change 551018 merged by jenkins-bot:
[mediawiki/extensions/WikimediaIncubator@master] Stop passing objects by reference

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

Change 551057 merged by jenkins-bot:
[mediawiki/extensions/FundraiserLandingPage@master] Stop passing objects by reference

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

Change 551056 merged by jenkins-bot:
[mediawiki/extensions/GeoCrumbs@master] Stop passing objects by reference

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

Change 551049 merged by jenkins-bot:
[mediawiki/extensions/GeoData@master] Stop passing objects by reference

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

Change 551048 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Stop passing objects by reference

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

Change 551047 merged by jenkins-bot:
[mediawiki/extensions/InputBox@master] Stop passing objects by reference

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

Change 551045 merged by jenkins-bot:
[mediawiki/extensions/Insider@master] Stop passing objects by reference

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

Change 551044 merged by jenkins-bot:
[mediawiki/extensions/intersection@master] Stop passing objects by reference

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

Change 551043 merged by jenkins-bot:
[mediawiki/extensions/Josa@master] Stop passing objects by reference

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

Change 551034 merged by jenkins-bot:
[mediawiki/extensions/LiquidThreads@master] Stop passing objects by reference

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

Change 551031 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Stop passing objects by reference

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

Change 551033 merged by jenkins-bot:
[mediawiki/extensions/MapSources@master] Stop passing objects by reference

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

Change 551028 merged by jenkins-bot:
[mediawiki/extensions/SiteMatrix@master] Stop passing objects by reference

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

Change 551029 merged by jenkins-bot:
[mediawiki/extensions/Scribunto@master] Stop passing objects by reference

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

Change 551025 merged by jenkins-bot:
[mediawiki/extensions/SubPageList3@master] Stop passing objects by reference

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

Change 551021 merged by jenkins-bot:
[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Stop passing objects by reference

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

Change 551020 merged by jenkins-bot:
[mediawiki/extensions/TemplateStyles@master] Stop passing objects by reference

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

Change 551019 merged by jenkins-bot:
[mediawiki/extensions/timeline@master] Stop passing objects by reference

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

Change 551015 merged by jenkins-bot:
[mediawiki/extensions/wikihiero@master] Stop passing objects by reference

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

Change 551255 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/FlaggedRevs@master] Stop passing objects by reference

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

Change 551257 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/PageAssessments@master] Stop passing objects by reference

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

Change 551261 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/QuizTabulate@master] Stop passing objects by reference

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

Change 551262 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Quiz@master] Stop passing objects by reference

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

Change 551276 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/WikimediaEvents@master] Stop passing objects by reference

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

Change 551277 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/XAnalytics@master] Stop passing objects by reference

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

Change 551279 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/WikimediaEditorTasks@master] Stop passing objects by reference

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

Change 551276 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Stop passing objects by reference

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

Change 551292 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Translate@master] Stop passing objects by reference

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

Change 551293 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/TorBlock@master] Stop passing objects by reference

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

Change 551294 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/TocTree@master] Stop passing objects by reference

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

Change 551295 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/TitleBlacklist@master] Stop passing objects by reference

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

Change 551296 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/TimedMediaHandler@master] Stop passing objects by reference

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

Change 551297 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/TheWikipediaLibrary@master] Stop passing objects by reference

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

Change 551295 merged by jenkins-bot:
[mediawiki/extensions/TitleBlacklist@master] Stop passing objects by reference

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

Change 551279 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEditorTasks@master] Stop passing objects by reference

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

Change 551257 merged by jenkins-bot:
[mediawiki/extensions/PageAssessments@master] Stop passing objects by reference

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

Change 551255 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Stop passing objects by reference

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

Change 551296 merged by jenkins-bot:
[mediawiki/extensions/TimedMediaHandler@master] Stop passing objects by reference

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

Change 551293 merged by jenkins-bot:
[mediawiki/extensions/TorBlock@master] Stop passing objects by reference

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

Change 551292 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Stop passing objects by reference

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

Change 551297 merged by Daimona Eaytoy:
[mediawiki/extensions/TheWikipediaLibrary@master] Stop passing objects by reference

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

Change 551567 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] hooks: Stop suggesting to pass objects by reference

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

Change 551570 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] hooks: Don't suggest to expect Article and User by reference

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

Change 551569 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/core@master] hooks: Do not document the Parser object to be passed by ref

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

Change 551294 merged by jenkins-bot:
[mediawiki/extensions/TocTree@master] Stop passing objects by reference

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

Change 551277 merged by jenkins-bot:
[mediawiki/extensions/XAnalytics@master] Stop passing objects by reference

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

Change 551567 merged by jenkins-bot:
[mediawiki/core@master] hooks: Stop suggesting to pass objects by reference

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

Change 551569 merged by jenkins-bot:
[mediawiki/core@master] hooks: Do not document the Parser object to be passed by ref

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

Change 551705 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/ConfirmEdit@master] Stop passing objects by reference

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

Change 551706 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/GrowthExperiments@master] Stop passing objects by reference

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

Change 551707 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/ProofreadPage@master] Stop passing objects by reference

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

Change 551710 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Echo@master] Stop passing objects by reference

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

Change 551262 merged by jenkins-bot:
[mediawiki/extensions/Quiz@master] Stop passing objects by reference

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

Change 551261 merged by jenkins-bot:
[mediawiki/extensions/QuizTabulate@master] Stop passing objects by reference

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

Change 551710 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Stop passing objects by reference

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

Change 551707 merged by jenkins-bot:
[mediawiki/extensions/ProofreadPage@master] Stop passing objects by reference

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

Change 551706 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Stop passing objects by reference

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

Change 551570 abandoned by Thiemo Kreuz (WMDE):
hooks: Don't suggest to expect Article and User by reference

Reason:
I will redo this later.

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

Change 551705 merged by jenkins-bot:
[mediawiki/extensions/ConfirmEdit@master] Stop passing objects by reference

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

For T240307 I really need a list of every inappropriate reference parameter. Ideally this would be indicated by some special syntax in hooks.txt, like [&]$editPage instead of &$editPage.

For T240307 I really need a list of every inappropriate reference parameter. Ideally this would be indicated by some special syntax in hooks.txt, like [&]$editPage instead of &$editPage.

I'm doing this.

Change 575407 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/core@master] Annotate hook parameters that are references for legacy reasons.

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

Change 575407 merged by jenkins-bot:
[mediawiki/core@master] Annotate hook parameters that are references for legacy reasons.

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

Change 583813 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/extensions/examples@master] Remove unnecessary references from ParserGetVariableValueSwitch hook

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

Change 583814 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/extensions/SpellingDictionary@master] Remove unnecessary references on ParserGetVariableValueSwitch hook

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

Change 583816 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/extensions/BlueSpiceFoundation@master] Remove unnecessary references on ParserGetVariableValueSwitch hook

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

Change 583817 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[translatewiki@master] Remove unnecessary references on ParserGetVariableValueSwitch hook

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

Change 583817 merged by jenkins-bot:
[translatewiki@master] Remove unnecessary references on ParserGetVariableValueSwitch hook

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

Change 583813 merged by jenkins-bot:
[mediawiki/extensions/examples@master] Remove unnecessary references from ParserGetVariableValueSwitch hook

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

Change 583814 merged by jenkins-bot:
[mediawiki/extensions/SpellingDictionary@master] Remove unnecessary references on ParserGetVariableValueSwitch hook

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

Change 583816 merged by jenkins-bot:
[mediawiki/extensions/BlueSpiceFoundation@master] Remove unnecessary references on ParserGetVariableValueSwitch hook

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

Umherirrender subscribed.

With the new hook system (T240307: Hook container with strong types and DI, HookContainer/HookRunner) it is now documented in the hook interface for what argument the pass-by-reference is needed. The signature of the interface is checked by php against the implementation. When a hook handler is changed to implement the interface it could be relevant to remove some of the pass-by-ref syntax in that case, but that is expected (and happens in the past).