Page MenuHomePhabricator

Checkup code passing $this by reference with a temporary variable
Open, Needs TriagePublic

Description

See this search for a list; this was briefly discussed on r539529.

Basically, &$this won't pass a reference to $this, hence there are two options:

  1. If you only want callees to modify properties of $this, the ampersand is redundant as of PHP4.
  2. If you expect the callee to replace the parameter with something else, you should assign $this to a temporary variable and pass that variable by reference. This is what the PHPCS sniff suggests, and the code in the search above is doing.

For PHPCS, we're probably going to suggest both (1.) and (2.) whenever we find &$this. Existing code should instead be checked manually, determine what its intent is, and act as appropriate.


Per T234118#5531469, Hooks::run calls should be audited manually, then we should write a script to update hook handlers. Handlers should be fixed first, then the Hooks::run calls.

Progress

(all hooks using a temp var to pass $this by reference must be fixed, as apparently there's no situation where we expect the param to change)

  • EditPage: reference can be removed from all hooks: EditPage::showEditForm:initial, EditPage::showEditForm:fields, EditPageBeforeConflictDiff, EditPageBeforeEditButtons, and EditPageNoSuchSection. Some handlers already updated, ref T234118#5531469.
  • OutputPage: reference can be removed from all hooks: OutputPageMakeCategoryLinks, OutputPageParserOutput, OutputPageBeforeHTML, and BeforePageDisplay.
  • Title: reference can be removed from all hooks: GetFullURL, GetLocalURL::Article, GetLocalURL::Internal, GetLocalURL, GetInternalURL, and GetCanonicalURL
  • RawAction: can be removed from RawPageViewBeforeOutput
  • HistoryPager: can be removed from PageHistoryPager::getQueryInfo
  • ApiBase: can be removed from APIGetAllowedParams
  • ApiPageSet: can be removed from APIQueryGeneratorAfterExecute
  • DatabaseBlock: can be removed from AbortAutoblock
  • ChangesList: can be removed from ChangesListInsertArticleLink
  • OldChangesList: can be removed from OldChangesListRecentChangesLine
  • RecentChange: can be removed from RecentChange_save
  • LinksUpdate: can be removed from LinksUpdateConstructed, LinksUpdate, and LinksUpdateComplete
  • DifferenceEngine: can be removed from AbortDiffCache
  • XmlDumpWriter: can be removed from XmlDumpWriterWriteRevision
  • LocalFile: can be removed from LocalFile::getHistory
  • Article: can be removed from ArticleAfterFetchContentObject, ArticleViewHeader, ArticleViewRedirect, DisplayOldSubtitle, and IsFileCacheable
  • CategoryPage: can be removed from CategoryPageView
  • ImagePage: can be removed from ImageOpenShowImageInlineBefore
  • WikiPage: can be removed from ArticlePageDataBefore, ArticlePageDataAfter, ArticleDelete, ArticlePurge, ArticleProtect, and ArticleProtectComplete
  • Parser: can be removed from: ParserFirstCallInit, ParserClearState, ParserBeforeStrip, ParserAfterStrip, ParserAfterParse, ParserBeforeInternalParse, InternalParseBeforeSanitize, InternalParseBeforeLinks, ParserAfterUnstrip, ParserBeforeTidy, ParserAfterTidy, ParserGetVariableValueVarCache, ParserGetVariableValueTs, ParserGetVariableValueSwitch, and BeforeParserrenderImageGallery. And also from setFunctionHook docs, mFunctionTagHooks, and mFunctionHooks
  • ResourceLoader: can be removed from ResourceLoaderTestModules
  • BaseTemplate: can be removed from BaseTemplateToolbox and SkinTemplateToolboxEnd
  • SkinTemplate: can be removed from SkinTemplateOutputPageBeforeExec, SkinTemplateTabAction, SkinTemplatePreventOtherActiveTabs, SkinTemplateNavigation, SkinTemplateNavigation::SpecialPage, SkinTemplateNavigation::Universal, and SkinTemplateBuildNavUrlsNav_urlsAfterPermalink
  • SpecialMovepage: can be removed from SpecialMovepageAfterMove
  • SpecialUpload: can be removed from SpecialUploadComplete, UploadForm:initial, and UploadForm:BeforeProcessing
  • SpecialWantedpages: can be removed from WantedPages::getQueryInfo
  • ContribsPager: can be removed from ContribsPager::getQueryInfo
  • NewPagesPager: can be removed from SpecialNewpagesConditions
  • UploadBase: can be removed from UploadComplete
  • User: can be removed from GetBlockedStatus, PingLimiter, UserIsBlockedGlobally, UserRetrieveNewTalks, UserEffectiveGroups, UserClearNewTalkNotification, UserLogout, UserCanSendEmail, and EmailConfirmed

Details

Related Gerrit Patches:
mediawiki/extensions/FlaggedRevs : masterRemove redundant reference from EditPage hook handler
mediawiki/extensions/BlueSpiceSaferEdit : REL1_31_devRemove redundant reference from EditPage hook handler
mediawiki/extensions/BlueSpiceSaferEdit : masterRemove redundant reference from EditPage hook handler
mediawiki/extensions/Genealogy : masterRemove redundant reference from EditPage hook handler
mediawiki/extensions/ConfirmEdit : masterRemove redundant reference from EditPage hook handler
mediawiki/extensions/Checkpoint : masterRemove redundant reference from EditPage hook handler
mediawiki/extensions/ForcePreview : masterRemove redundant reference from EditPage hook handler
mediawiki/extensions/ApprovedRevs : masterRemove redundant reference from EditPage hook handler

Event Timeline

Daimona created this task.Sep 28 2019, 9:31 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 28 2019, 9:31 AM

Note that we should first change the hook handlers and remove the references there.

Change 539654 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/ApprovedRevs@master] Remove redundant reference from EditPage hook handler

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

Change 539655 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/BlueSpiceSaferEdit@master] Remove redundant reference from EditPage hook handler

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

Change 539656 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/Checkpoint@master] Remove redundant reference from EditPage hook handler

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

Change 539657 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/ConfirmEdit@master] Remove redundant reference from EditPage hook handler

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

Change 539658 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/FlaggedRevs@master] Remove redundant reference from EditPage hook handler

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

Change 539659 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/ForcePreview@master] Remove redundant reference from EditPage hook handler

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

Change 539660 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/Genealogy@master] Remove redundant reference from EditPage hook handler

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

Daimona added a comment.EditedSep 28 2019, 10:14 AM

EditPage

No hook needs the reference.
Progress:

--> And at this point I'm already bored. We're not even halfway through for EditPage itself: https://codesearch.wmflabs.org/things/?q=EditPage(%3A%3A%7CBefore%7CNoSuch)&i=nope&files=extension.json&repos=

I think we should audit hook calls manually, then set up a script to find hook handlers for those hooks and remove the ampersands.

Daimona updated the task description. (Show Details)Sep 28 2019, 11:56 AM

Change 539659 merged by jenkins-bot:
[mediawiki/extensions/ForcePreview@master] Remove redundant reference from EditPage hook handler

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

Change 539656 merged by jenkins-bot:
[mediawiki/extensions/Checkpoint@master] Remove redundant reference from EditPage hook handler

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

Change 539657 merged by jenkins-bot:
[mediawiki/extensions/ConfirmEdit@master] Remove redundant reference from EditPage hook handler

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

Change 539660 merged by jenkins-bot:
[mediawiki/extensions/Genealogy@master] Remove redundant reference from EditPage hook handler

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

Change 539846 had a related patch set uploaded (by Robert Vogel; owner: Daimona Eaytoy):
[mediawiki/extensions/BlueSpiceSaferEdit@REL1_31_dev] Remove redundant reference from EditPage hook handler

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

Change 539655 merged by Robert Vogel:
[mediawiki/extensions/BlueSpiceSaferEdit@master] Remove redundant reference from EditPage hook handler

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

Change 539846 merged by Robert Vogel:
[mediawiki/extensions/BlueSpiceSaferEdit@REL1_31_dev] Remove redundant reference from EditPage hook handler

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

Change 539658 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Remove redundant reference from EditPage hook handler

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