Page MenuHomePhabricator

Checkup code passing $this by reference with a temporary variable
Closed, ResolvedPublic

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

Event Timeline

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

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.

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

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

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

Question: UserRetrieveNewTalks passes the User by reference. But, looking at callers, it doesn't appear to have any - should the hook be removed?

UserRetrieveNewTalks […] doesn't appear to have any [callers]. Should the hook be removed?

Yes, absolutely. Please remove hooks that don't have any callers. If anybody needs a similar hook in the future, we can easily introduce one again.

I'm not absolutely sure how we apply our deprecation policies in such a situation. I believe it's fine to remove it without deprecation. That how it was done in other situations, e.g. https://gerrit.wikimedia.org/r/566686.

UserRetrieveNewTalks […] doesn't appear to have any [callers]. Should the hook be removed?

Yes, absolutely. Please remove hooks that don't have any callers. If anybody needs a similar hook in the future, we can easily introduce one again.

I'm not absolutely sure how we apply our deprecation policies in such a situation. I believe it's fine to remove it without deprecation. That how it was done in other situations, e.g. https://gerrit.wikimedia.org/r/566686.

T248651: 20% of hooks appear to be unused came to a different conclusion

Umherirrender subscribed.

the codesniffer part was done with T169168 and with the new hook system in 1.36 there was many fixes as well.

Assuming this is already fixed/resolved