Page MenuHomePhabricator
Create Task
Maniphest T153505

Use of &$this in hooks triggers warnings in PHP 7.1
Closed, ResolvedPublic
Actions

Assigned To
None
Authored By
Legoktm
Dec 16 2016, 10:24 PM
Tags
Referenced Files
None
Subscribers
Aklapper
amritsreekumar
Florian
gerritbot
Isarra
jayvdb
Legoktm
View All 23 Subscribers
Tokens
"Heartbreak" token, awarded by thiemowmde."Like" token, awarded by Liuxinyu970226."Like" token, awarded by xSavitar.

Description

Via @mwjames:

Code like Hooks::run( 'WantedPages::getQueryInfo', [ &$this, &$query ] ); will trigger warnings in PHP 7.1. Ideally the solution would be to remove &, which hasn't been needed since PHP 4, however that breaks backwards-compatibility.

Instead, we can rename $this to a different variable, and then pass it by reference. Example:

		// Avoid PHP 7.1 warning of passing $this by reference
		$editPage = $this;
		Hooks::run( 'EditPage::showEditForm:initial', [ &$editPage, &$out ] );

See https://bugs.php.net/bug.php?id=73751 / https://wiki.php.net/rfc/this_var

Example commit to fix this issue: https://gerrit.wikimedia.org/r/#/c/328387/

More information:

Details

SubjectRepoBranchLines +/-
Replace the use of &$thismediawiki/skins/apexmaster+1 -1
Replace the use of &$thismediawiki/skins/Splashmaster+1 -1
Avoid PHP 7.1 warning of passing $this by referencemediawiki/skins/VectorV2master+3 -2
Avoid PHP 7.1 warning of passing $this by referencemediawiki/skins/Synagonismmaster+3 -1
Avoid PHP 7.1 warning of passing $this by referencemediawiki/extensions/InterwikiIntegrationmaster+9 -3
Avoid PHP 7.1 warning of passing $this by referencemediawiki/extensions/MultiUploadmaster+3 -1
Avoid PHP 7.1 warning of passing $this by referencemediawiki/skins/p2wikimaster+3 -1
Replace the use of &$thismediawiki/skins/Duskmaster+1 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/DeskMessMirroredmaster+3 -2
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/Truglassmaster+3 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/apexmaster+3 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/Bouquetmaster+3 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/DuskToDawnmaster+3 -2
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/WPtouchmaster+3 -2
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/eruditemaster+3 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/Gamepressmaster+3 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/Refreshedmaster+3 -1
Avoid PHP 7.1 warning of &$this usagemediawiki/skins/Duskmaster+3 -1
Changed &$this in hooksmediawiki/extensions/NewUserNotifmaster+6 -2
Changed &$this in hooksmediawiki/skins/webplatformmaster+4 -2
UserProfilePage: Avoid &$this in hooksmediawiki/extensions/SocialProfilemaster+7 -4
SpecialGlobalContributions: Avoid &$this in hooksmediawiki/extensions/GlobalContribsmaster+3 -1
Replace the use of &$thismediawiki/skins/Vectormaster+1 -1
Don't pass $this by referencemediawiki/skins/VectorREL1_29+3 -1
Don't pass $this by referencemediawiki/skins/VectorREL1_28+3 -1
Don't pass $this by referencemediawiki/skins/VectorREL1_27+3 -1
Don't pass $this by referencemediawiki/skins/MonoBookREL1_29+5 -2
Don't pass $this by referencemediawiki/skins/MonoBookREL1_28+5 -2
Don't pass $this by referencemediawiki/skins/MonoBookREL1_27+5 -2
Don't pass $this by referencemediawiki/skins/Vectormaster+5 -4
Don't pass $this by referencemediawiki/skins/MonoBookmaster+4 -3
upload: Avoid &$this in hooksmediawiki/coreREL1_27+12 -5
Remove &$this usagemediawiki/coreREL1_27+33 -12
upload: Avoid &$this in hooksmediawiki/coremaster+12 -5
Avoid passing $this by reference in hooksmediawiki/coreREL1_27+18 -6
Replace some usages of &$this in hook parametersmediawiki/coreREL1_27+22 -6
Replace &$this with $this in MediaWiki/core hook systemmediawiki/coreREL1_27+56 -17
Avoid passing $this by reference in hooksmediawiki/coremaster+18 -6
Replace some usages of &$this in hook parametersmediawiki/coremaster+22 -6
Replaced &$this with $thismediawiki/coreREL1_27+25 -11
EditPage: Avoid use of &$this in hooksmediawiki/coreREL1_27+19 -6
Replaced &$this with $thismediawiki/coremaster+25 -11
EditPage: Avoid use of &$this in hooksmediawiki/coreREL1_28+20 -6
EditPage: Avoid use of &$this in hooksmediawiki/coremaster+20 -6
EditPage: Avoid use of &$this in hooksmediawiki/coreREL1_20+20 -6
Work around &$this usage in SkinTemplatemediawiki/coreREL1_27+21 -7
Replace the use of &$thismediawiki/extensions/Wikilogmaster+3 -3
Replace the use of &$thismediawiki/extensions/Contestmaster+4 -5
Compatibility: Replaced &$this with $thismediawiki/extensions/BlueSpiceExtensionsREL1_27+28 -28
Compatibility: Replaced &$this with $thismediawiki/extensions/BlueSpiceExtensionsmaster+28 -28
Replaced &$this with $thismediawiki/extensions/BlueSpiceFoundationREL1_27+2 -2
Replaced &$this with $thismediawiki/extensions/BlueSpiceFoundationmaster+2 -2
Replace use of &$thismediawiki/coreREL1_28+126 -57
Work around &$this usage in SkinTemplatemediawiki/coreREL1_28+21 -7
Replace use of &$thismediawiki/coreREL1_27+125 -56
Work around &$this usage in SkinTemplatemediawiki/coremaster+21 -7
Modernise, desoupify, and refactor everythingmediawiki/skins/GreyStuffmaster+605 -406
Replace the use of &$thismediawiki/extensions/BlueSpiceFoundationmaster+3 -3
Replace use of &$thismediawiki/coremaster+123 -57
Replace use of &$thismediawiki/extensions/ConfirmEditREL1_28+1 -1
Replace use of &$thismediawiki/extensions/ConfirmEditREL1_27+1 -1
Replace use of &$thismediawiki/extensions/MathREL1_28+9 -3
Replace use of &$thismediawiki/extensions/MathREL1_27+9 -3
Replace use of &$thismediawiki/extensions/Mathmaster+9 -3
Replace use of &$thismediawiki/extensions/ConfirmEditmaster+1 -1
Replace use of &$thismediawiki/extensions/PageFormsmaster+12 -4
Replace &$this with $thismediawiki/coremaster+0 -0
Replace &$this with $this in MediaWiki/core hook systemmediawiki/coremaster+66 -18
Remove &$this usagemediawiki/coremaster+33 -11
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
gerritbot added a comment.Sep 1 2017, 2:29 PM

Change 375383 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/skins/MonoBook@REL1_29] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:39 PM

Change 375113 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:39 PM

Change 375381 merged by jenkins-bot:
[mediawiki/skins/MonoBook@REL1_27] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:40 PM

Change 375382 merged by jenkins-bot:
[mediawiki/skins/MonoBook@REL1_28] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:40 PM

Change 375383 merged by jenkins-bot:
[mediawiki/skins/MonoBook@REL1_29] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:41 PM

Change 375385 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/skins/Vector@REL1_27] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:41 PM

Change 375386 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/skins/Vector@REL1_28] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:42 PM

Change 375387 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/skins/Vector@REL1_29] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:44 PM

Change 375385 merged by jenkins-bot:
[mediawiki/skins/Vector@REL1_27] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:46 PM

Change 375386 merged by jenkins-bot:
[mediawiki/skins/Vector@REL1_28] Don't pass $this by reference

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

gerritbot added a comment.Sep 1 2017, 2:50 PM

Change 375387 merged by jenkins-bot:
[mediawiki/skins/Vector@REL1_29] Don't pass $this by reference

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

Ciencia_Al_Poder unsubscribed.Sep 1 2017, 3:04 PM
Aklapper moved this task from Doing to Backlog on the good first task board.Oct 16 2017, 2:36 PM
gerritbot added a comment.Oct 23 2017, 11:36 PM

Change 341793 abandoned by Krinkle:
Replace the use of &$this

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

Framawiki moved this task from Backlog to Doing on the good first task board.Dec 2 2017, 1:29 PM
Reedy removed projects: MW-1.30-release-notes (WMF-deploy-2017-09-05 (1.30.0-wmf.17)), Patch-For-Review, Google-Code-In-2016.Dec 8 2017, 3:28 AM
Reedy added projects: BlueSpice, GlobalContribs, MediaWiki-extensions-Other, MediaWiki-extensions-MultiUpload, MediaWiki-extensions-NewUserNotif, SocialProfile, MediaWiki-extensions-SwiftCloudFiles, Apex, Bouquet, DeepSea, DeskMessMirrored, Dusk, DuskToDawn, Refreshed, MediaWiki-skins-Slate, Truglass.Dec 8 2017, 3:35 AM

Some, but not many to go!

reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki$ find extensions skins -name \*.php -exec grep --perl-regexp '&\$this(?!->|\[)' {} + | gawk -F/ '{print $1 FS $2}' | sort | uniq -c | sed 's/^ *\([0-9]\+\) \(.*\)/* \2 (\1 uses)/;s/(1 uses)/(1 use)/;'
* extensions/BlueSpiceExtendedSearch (2 uses)
* extensions/GlobalContribs (1 use)
* extensions/InterwikiIntegration (3 uses)
* extensions/MSSQLBackCompat (2 uses)
* extensions/MultiUpload (1 use)
* extensions/NewUserNotif (2 uses)
* extensions/SemanticPageMaker (4 uses)
* extensions/SocialProfile (4 uses)
* extensions/SwiftCloudFiles (4 uses)
* skins/apex (1 use)
* skins/Bouquet (1 use)
* skins/DeepSea (1 use)
* skins/DeskMessMirrored (1 use)
* skins/Dusk (1 use)
* skins/DuskToDawn (1 use)
* skins/erudite (1 use)
* skins/Gamepress (1 use)
* skins/p2wiki (1 use)
* skins/Refreshed (1 use)
* skins/Slate (1 use)
* skins/Synagonism (1 use)
* skins/Truglass (1 use)
* skins/VectorV2 (1 use)
* skins/webplatform (2 uses)
* skins/WPtouch (1 use)
Restricted Application added a project: Social-Tools. · View Herald TranscriptDec 8 2017, 3:35 AM
Reedy added a comment.Dec 8 2017, 3:53 AM

And possibly some false positives here?

Certainly looks to be for BlueSpiceExtendedSearch

Reedy added a comment.Dec 8 2017, 3:56 AM
reedy@ubuntu64-web-esxi:/var/www/wiki/mediawiki$ find extensions skins -name \*.php -exec grep --perl-regexp '&\$this(?!->|\[)' {} +
extensions/InterwikiIntegration/InterwikiIntegrationRecentChange.php:		Hooks::run( 'RecentChange_save', array( &$this ) );
extensions/InterwikiIntegration/InterwikiIntegrationChangesList.php:			array(&$this, &$articlelink, &$s, &$rc, $unpatrolled, $watched) );
extensions/InterwikiIntegration/InterwikiIntegrationChangesList.php:		Hooks::run( 'OldInterwikiIntegrationChangesListRecentChangesLine', array(&$this, &$s, $rc) );
extensions/MultiUpload/SpecialMultiUpload.php:			if ( !Hooks::run( 'UploadForm:initial', array( &$this ) ) ) {
extensions/MSSQLBackCompat/DatabaseMssqlOld.php:				$from = ' FROM ' . implode( ',', array_map( array( &$this, 'tableName' ), $table ) );
extensions/MSSQLBackCompat/DatabaseMssqlOld.php:			$srcTable =  implode( ',', array_map( array( &$this, 'tableName' ), $srcTable ) );
extensions/GlobalContribs/SpecialGlobalContributions.php:		Hooks::run( 'ContribsPager::getQueryInfo', array( &$this, &$queryInfo ) );
extensions/SwiftCloudFiles/php-cloudfiles-wmf/cloudfiles_http.php:		curl_setopt( $curl_ch, CURLOPT_HEADERFUNCTION, array( &$this, '_auth_hdr_cb' ) );
extensions/SwiftCloudFiles/php-cloudfiles-wmf/cloudfiles_http.php:		curl_setopt( $ch, CURLOPT_HEADERFUNCTION, array( &$this, '_header_cb' ) );
extensions/SwiftCloudFiles/php-cloudfiles-wmf/cloudfiles_http.php:			curl_setopt( $ch, CURLOPT_WRITEFUNCTION, array( &$this, '_write_cb' ) );
extensions/SwiftCloudFiles/php-cloudfiles-wmf/cloudfiles_http.php:			curl_setopt( $ch, CURLOPT_READFUNCTION, array( &$this, '_read_cb' ) );
extensions/BlueSpiceExtendedSearch/src/Lookup.php:		$aBase = &$this;
extensions/BlueSpiceExtendedSearch/src/Lookup.php:		$aBase = &$this;
extensions/SocialProfile/UserProfile/UserProfilePage.php:		if ( !Hooks::run( 'UserProfileBeginLeft', array( &$this ) ) ) {
extensions/SocialProfile/UserProfile/UserProfilePage.php:		if ( !Hooks::run( 'UserProfileEndLeft', array( &$this ) ) ) {
extensions/SocialProfile/UserProfile/UserProfilePage.php:		if ( !Hooks::run( 'UserProfileBeginRight', array( &$this ) ) ) {
extensions/SocialProfile/UserProfile/UserProfilePage.php:		if ( !Hooks::run( 'UserProfileEndRight', array( &$this ) ) ) {
extensions/SemanticPageMaker/specials/WidgetAssembler/SPM_UploadWindow2.php:		wfRunHooks( 'SpecialUploadComplete', array( &$this ) );
extensions/SemanticPageMaker/specials/WidgetAssembler/SPM_UploadWindow.php:		if ( !wfRunHooks( 'UploadForm:BeforeProcessing', array( &$this ) ) )
extensions/SemanticPageMaker/specials/WidgetAssembler/SPM_UploadWindow.php:		if ( !wfRunHooks( 'UploadForm:initial', array( &$this ) ) )
extensions/SemanticPageMaker/includes/widgets/SPM_WidgetPage.php:		if ( !wfRunHooks( 'SPMWidgetPageView', array( &$this ) ) )
extensions/NewUserNotif/NewUserNotif.class.php:		Hooks::run( 'NewUserNotifSubject', array( &$this, &$subjectLine, $wgSitename, $recipient, $user ) );
extensions/NewUserNotif/NewUserNotif.class.php:		Hooks::run( 'NewUserNotifBody', array( &$this, &$messageBody, $wgSitename, $recipient, $user ) );
skins/VectorV2/VectorV2Template.php:							Hooks::run( $hook, [ &$this, true ] );
skins/Dusk/includes/DuskTemplate.php:		Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/Gamepress/includes/GamepressTemplate.php:		Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/erudite/Erudite.skin.php:					Hooks::run( 'SkinTemplateToolboxEnd', array( &$this ) );
skins/Refreshed/includes/RefreshedTemplate.php:							Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/WPtouch/WPtouch.skin.php:				Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/Synagonism/SkinSynagonism.php:				wfRunHooks( $hook, array( &$this, true ) );
skins/webplatform/WebPlatformTemplate.class.php:							<?php Hooks::run( 'SkinBreadcrumb', [ &$this ] ); ?>
skins/webplatform/WebPlatformTemplate.class.php:						<?php /* wfRunHooks( 'SkinTOC', [ &$this ] );*/ ?>
skins/DuskToDawn/includes/DuskToDawnTemplate.php:		Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/Bouquet/includes/BouquetTemplate.php:		Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/p2wiki/P2wiki.skin.php:			wfRunHooks( 'SkinTemplateToolboxEnd', array( &$this ) );
skins/Slate/SlateTemplate.php:							wfRunHooks( $hook, array( &$this, true ) );
skins/apex/ApexTemplate.php:				Hooks::run( $hook, [ &$this, true ] );
skins/DeepSea/DeepSea.skin.php:				Hooks::run( $hook, array( &$this, true ) );
skins/Truglass/includes/TruglassTemplate.php:		Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
skins/DeskMessMirrored/includes/DeskMessMirroredTemplate.php:		Hooks::run( 'SkinTemplateToolboxEnd', array( &$this, true ) );
Reedy added a project: Google-Code-in-2017.Dec 8 2017, 3:56 AM
Anomie added a comment.EditedDec 8 2017, 5:45 PM
In T153505#3822042, @Reedy wrote:

And possibly some false positives here?

Certainly looks to be for BlueSpiceExtendedSearch

In T153505#3822043, @Reedy wrote:
extensions/BlueSpiceExtendedSearch/src/Lookup.php:		$aBase = &$this;
extensions/BlueSpiceExtendedSearch/src/Lookup.php:		$aBase = &$this;

While this doesn't result in a warning, it also doesn't result in $aBase containing a reference. Unless a supported version of PHP (i.e. not PHP4) requires a reference there, the ampersand may as well be removed for cleanliness.

The same goes for all the examples where &$this is used to make a callable rather than being passed as an argument to a hook.

Umherirrender removed a project: MediaWiki-extensions-SwiftCloudFiles.Dec 8 2017, 7:42 PM
Umherirrender unsubscribed.
Osnard mentioned this in rEBSESd7080349bbf0: Remove &$this occurences.Dec 11 2017, 7:09 AM
Aklapper removed amritsreekumar as the assignee of this task.Dec 15 2017, 10:13 AM
Aklapper updated the task description. (Show Details)
Aklapper added a subscriber: amritsreekumar.

@amritsreekumar: I'm going to remove you as assignee here, as there is more work to do and we're thinking of offering this task in Google Code-in 2017 to students. Thanks for all your patches and help in March!

Aklapper moved this task from Proposed tasks to Imported in GCI Site on the Google-Code-in-2017 board.Dec 15 2017, 10:37 AM

Imported those repos I consider maintained as Google-Code-in-2017 tasks:

gerritbot added a comment.Dec 17 2017, 5:06 AM

Change 398672 had a related patch set uploaded (by Rafidaslam; owner: Rafid Aslam):
[mediawiki/extensions/GlobalContribs@master] SpecialGlobalContributions: Avoid &$this in hooks

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

gerritbot added a project: Patch-For-Review.Dec 17 2017, 5:06 AM

Change 398673 had a related patch set uploaded (by Rafidaslam; owner: Rafid Aslam):
[mediawiki/extensions/SocialProfile@master] UserProfilePage: Avoid &$this in hooks

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

rafidaslam mentioned this in rEGCNa32806db44ce: SpecialGlobalContributions: Avoid &$this in hooks.Dec 17 2017, 5:39 AM
jayvdb subscribed.Dec 19 2017, 5:16 AM

@Reedy , the task description say " The solution is to remove the & " and

So basically this is just about removing one character, but you should check if the reference is actually needed/used. In nearly all cases, & is not needed.

However most of the examples are creating and using a new variable.
Should the task description be updated to mention this alternative solution?
And explain when each approach should be used?

Legoktm updated the task description. (Show Details)Dec 19 2017, 6:06 AM

The task description was wrong, I've updated it.

gerritbot added a comment.Dec 19 2017, 6:09 AM

Change 398672 merged by jenkins-bot:
[mediawiki/extensions/GlobalContribs@master] SpecialGlobalContributions: Avoid &$this in hooks

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

gerritbot added a comment.Dec 19 2017, 6:09 AM

Change 398673 merged by jenkins-bot:
[mediawiki/extensions/SocialProfile@master] UserProfilePage: Avoid &$this in hooks

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

jayvdb added a comment.Dec 19 2017, 8:19 AM

The task description was wrong, I've updated it.

Thanks. I've updated the GCI tasks to match your modifications, and published the other four tasks in this series as they should be good to go now that we've got the description QA'd ;-)

Aklapper added a comment.Dec 19 2017, 9:45 AM

Thank you!

gerritbot added a comment.Dec 22 2017, 12:14 AM

Change 399762 had a related patch set uploaded (by Ryan10145; owner: Ryan10145):
[mediawiki/skins/webplatform@master] Changed &$this in hooks

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

gerritbot added a comment.Dec 22 2017, 12:20 AM

Change 399763 had a related patch set uploaded (by Ryan10145; owner: Ryan10145):
[mediawiki/extensions/NewUserNotif@master] Changed &$this in hooks

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

gerritbot added a comment.Dec 22 2017, 10:49 PM

Change 399763 merged by jenkins-bot:
[mediawiki/extensions/NewUserNotif@master] Changed &$this in hooks

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

gerritbot added a comment.Dec 22 2017, 10:50 PM

Change 399762 merged by jenkins-bot:
[mediawiki/skins/webplatform@master] Changed &$this in hooks

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

lilydjwg unsubscribed.Dec 23 2017, 3:28 AM
Liuxinyu970226 awarded a token.Dec 23 2017, 4:47 AM
thiemowmde subscribed.Dec 23 2017, 9:18 AM

I'm very much disappointed by the changes done here (as already expressed in the discussion at e.g. https://gerrit.wikimedia.org/r/399763). All these &$this are just wrong, and always have been wrong. They must all be changed to $this, and all hook handlers repeating the same error must be fixed.

What the patches do instead is introducing a local variable that does nothing. This is not fixing anything. This is actually making the situation worse.

Please stop making all this code more obscure and harder to maintain in the future.

thiemowmde awarded a token.Dec 23 2017, 9:19 AM
Anomie added a comment.Dec 23 2017, 11:16 PM
In T153505#3858659, @thiemowmde wrote:

All these &$this are just wrong, and always have been wrong.

It seems you're unaware of PHP history.

Back in PHP4, objects were passed by value instead of by reference. To avoid a copy, the function parameter had to be a reference. Even when $this was being passed. And, of course, call_user_func_array() needs to explicitly take a reference of parameters for which the target function is expecting a reference.

So, no, it hasn't "always been wrong".

Also, for that matter, back in PHP4 it was in fact legal to reassign $this. That sort of thing is one of many reasons why people hated PHP back then. Directly reassigning $this was prohibited in PHP 5, but even in 7.0.26 if you took a reference to $this then you could reassign it as you could any other reference. PHP 7.1 finally fixed that one.

They must all be changed to $this, and all hook handlers repeating the same error must be fixed.

Breaking every existing hook handler that takes a reference for a parameter where the hook caller would pass $this would be a rather large breaking change. Are you volunteering to take on that project? Will you do it in compliance with the deprecation policy?

What the patches do instead is introducing a local variable that does nothing.

It allows a reference to be passed to hook handlers that expect it, rather than breaking every such handler. That's not "nothing".

thiemowmde added a comment.Dec 24 2017, 11:12 AM

I started coding in PHP 3 and are aware of the history.

I'm afraid you did not got my argument. Not only is $thisShouldNotBeAReference = $this; […] &$thisShouldNotBeAReference semantically the same as &$this. It's worse, for a multitude of reasons. Nobody can systematically search and fix these places, now that they don't contain &$this any more. The code that is now left behind is confusing, bogus, and fragile. Any developer later looking at this code will wonder a) what the point of the useless variable is, and b) what the point of passing $this by reference is, if there is no code that does anything with this variable after the hook was triggered. People will remove the useless variable or the & and run into the unsolved issue again. Because the patches did not solved it. Also there is no need to change it the way as done in these patches, as it does not block us from using PHP 7. It's just a warning. All the patches do is hiding a systemic issue in the code.

The least minimal next step I expect to see here is a list of hooks that misuse references to $this, and ideally individual tickets for each of them to be fixed in the future.

Legoktm added a comment.Dec 24 2017, 7:18 PM
In T153505#3859627, @thiemowmde wrote:

Nobody can systematically search and fix these places, now that they don't contain &$this any more.

Sure you can. http://codesearch.wmflabs.org/search/?q=Avoid%20PHP%207.1%20warning%20from%20passing%20%5C%24this%20by%20reference&i=nope&files=&repos=

Anomie added a comment.Dec 24 2017, 7:56 PM
In T153505#3859627, @thiemowmde wrote:

The code that is now left behind is confusing,

Maybe it would be, if we weren't commenting it to explain exactly why it's being done.

bogus,

Not really.

and fragile.

Not at all.

Any developer later looking at this code will wonder a) what the point of the useless variable is,

Only if they habitually ignore comments.

and b) what the point of passing $this by reference is, if there is no code that does anything with this variable after the hook was triggered.

That they might wonder. But,

People will remove the useless variable or the & and run into the unsolved issue again.

and then they will be enlightened. Or more likely, they'll be enlightened in code review, or if they just ask anyone who's been around long enough.

The least minimal next step I expect to see here is a list of hooks that misuse references to $this, and ideally individual tickets for each of them to be fixed in the future.

You know how to work Phabricator. But don't expect everyone else to work on your pet peeve.

In T153505#3859995, @Legoktm wrote:
In T153505#3859627, @thiemowmde wrote:

Nobody can systematically search and fix these places, now that they don't contain &$this any more.

Sure you can. http://codesearch.wmflabs.org/search/?q=Avoid%20PHP%207.1%20warning%20from%20passing%20%5C%24this%20by%20reference&i=nope&files=&repos=

And, for that matter, http://codesearch.wmflabs.org/search/?q=%5B%20%5D%3D%20%5C%24this%3B&i=nope&files=&repos= doesn't seem to have too many false positives (and this one is probably even better).

And as a bonus, that search identifies some pre-PHP5.4 code that could be cleaned up too.

thiemowmde added a comment.Dec 25 2017, 1:13 PM

[…] if we weren't commenting it […]

Comments don't solve the issue here. Comments can give reasons, but they can not magically remove the confusion from confusing code.

[…] and fragile.

Not at all.

So you did not got my point. I'm sorry.

[…] if they just ask anyone who's been around long enough.

I hope you realize how flawed this advice is. Code must speak for itself. You can't rely on some experts to be available forever. There are entire books about the systemic issues that come with such an attitude.

[…] don't expect everyone else to work on your pet peeve.

Please check your tone and attitude. Thanks.

Have a nice holiday.

gerritbot added a comment.Dec 27 2017, 9:21 PM

Change 400441 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/Dusk@master] Moved the $this references into variables

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

gerritbot added a comment.Dec 28 2017, 9:29 AM

Change 400441 merged by jenkins-bot:
[mediawiki/skins/Dusk@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 28 2017, 9:35 AM

Change 400562 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/erudite@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 28 2017, 9:38 AM

Change 400563 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/Gamepress@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 29 2017, 1:11 PM

Change 400693 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/Refreshed@master] Avoid PHP 7.1 warning of &$this usage

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

LukBukkit mentioned this in R1893:f9d5fc3b5b0c: Avoid PHP 7.1 warning of &$this usage.Dec 29 2017, 1:52 PM
gerritbot added a comment.Dec 29 2017, 8:08 PM

Change 400693 merged by jenkins-bot:
[mediawiki/skins/Refreshed@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 29 2017, 8:09 PM

Change 400563 merged by jenkins-bot:
[mediawiki/skins/Gamepress@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 29 2017, 8:09 PM

Change 400562 merged by jenkins-bot:
[mediawiki/skins/erudite@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 29 2017, 9:46 PM

Change 400717 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/WPtouch@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 29 2017, 9:48 PM

Change 400718 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/DuskToDawn@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 29 2017, 9:51 PM

Change 400719 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/Bouquet@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 3:03 PM

Change 400717 merged by jenkins-bot:
[mediawiki/skins/WPtouch@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 3:03 PM

Change 400718 merged by jenkins-bot:
[mediawiki/skins/DuskToDawn@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 3:03 PM

Change 400719 merged by jenkins-bot:
[mediawiki/skins/Bouquet@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 8:45 PM

Change 401095 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/apex@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 8:46 PM

Change 401096 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/Truglass@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 8:48 PM

Change 401097 had a related patch set uploaded (by LukBukkit; owner: LukBukkit):
[mediawiki/skins/DeskMessMirrored@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 8:57 PM

Change 401095 merged by jenkins-bot:
[mediawiki/skins/apex@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 8:57 PM

Change 401096 merged by jenkins-bot:
[mediawiki/skins/Truglass@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Dec 30 2017, 8:57 PM

Change 401097 merged by jenkins-bot:
[mediawiki/skins/DeskMessMirrored@master] Avoid PHP 7.1 warning of &$this usage

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

gerritbot added a comment.Jan 6 2018, 1:16 AM

Change 341811 abandoned by SamanthaNguyen:
Replace the use of &$this

Reason:
This has been correctly fixed in I4dff3f65457682d87e2954f5b0f66b4ffc2f82b4

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

gerritbot added a comment.Feb 14 2018, 7:41 PM

Change 410539 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/skins/p2wiki@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 14 2018, 7:45 PM

Change 410541 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/MultiUpload@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 14 2018, 7:50 PM

Change 410544 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/InterwikiIntegration@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 14 2018, 7:56 PM

Change 410547 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/skins/Synagonism@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 14 2018, 7:56 PM

Change 410548 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/skins/VectorV2@master] Avoid PHP 7.1 warning of passing $this by reference

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

Umherirrender mentioned this in rSVEV9b581ee1dce1: Avoid PHP 7.1 warning of passing $this by reference.Feb 14 2018, 7:58 PM
Umherirrender added a comment.Feb 14 2018, 7:58 PM

Searched with https://codesearch.wmflabs.org/search/?q=%26%5C%24this%5Cs*%5B%5E-%5Cs%5C%5B%5D&i=nope&files=%5C.php&repos=

Uploaded patch set for the extensions/skins found there. Should be resolved when the patches are merged.
One extension in the list is in request for archive

Umherirrender mentioned this in rSVEVb104b4b1c28c: Avoid PHP 7.1 warning of passing $this by reference.Feb 15 2018, 7:20 PM
gerritbot added a comment.Feb 16 2018, 4:47 PM

Change 410539 merged by jenkins-bot:
[mediawiki/skins/p2wiki@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 16 2018, 4:48 PM

Change 410541 merged by jenkins-bot:
[mediawiki/extensions/MultiUpload@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 16 2018, 4:48 PM

Change 410544 merged by jenkins-bot:
[mediawiki/extensions/InterwikiIntegration@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 16 2018, 4:48 PM

Change 410547 merged by jenkins-bot:
[mediawiki/skins/Synagonism@master] Avoid PHP 7.1 warning of passing $this by reference

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

gerritbot added a comment.Feb 16 2018, 4:48 PM

Change 410548 merged by jenkins-bot:
[mediawiki/skins/VectorV2@master] Avoid PHP 7.1 warning of passing $this by reference

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

Umherirrender closed this task as Resolved.Feb 16 2018, 4:55 PM
Liuxinyu970226 unsubscribed.Feb 17 2018, 1:20 AM
gerritbot added a comment.Mar 16 2018, 9:20 PM

Change 341800 abandoned by Isarra:
Replace the use of &$this

Reason:
Issue was otherwise resolved.

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

Umherirrender mentioned this in rSVEVe0ce75253956: Final NoteDb migration updates.Jun 11 2018, 11:27 AM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:42 PM
Restricted Application added a subscriber: MacFan4000. · View Herald TranscriptOct 16 2020, 5:42 PM
gerritbot added a comment.Oct 30 2020, 4:29 PM

Change 341526 abandoned by Jdlrobson:
[mediawiki/skins/apex@master] Replace the use of &$this

Reason:
This issue was resolved in https://gerrit.wikimedia.org/r/c/mediawiki/skins/apex/ /401095. Amritsreekumar I apologize that your patch was neglected here.

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

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL