Page MenuHomePhabricator

PHP error from Special:OAuth: "Indirect modification of overloaded property User::$oAuthUserData"
Closed, ResolvedPublic

Description

Error

Request URL: /wiki/Special:OAuth/authorize
Request ID: XSYLYgpAAD0AAC9KeaYAAACE

message
PHP Notice: Indirect modification of overloaded property User::$oAuthUserData has no effect
trace
#0 /srv/mediawiki/php-1.34.0-wmf.13/extensions/OAuth/includes/backend/MWOAuthUtils.php(295): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.34.0-wmf.13/extensions/OAuth/includes/backend/MWOAuthServer.php(445): MediaWiki\Extensions\OAuth\MWOAuthUtils::getCentralIdFromLocalUser(User)
#2 /srv/mediawiki/php-1.34.0-wmf.13/extensions/OAuth/includes/frontend/specialpages/SpecialMWOAuth.php(369): MediaWiki\Extensions\OAuth\MWOAuthServer->getCurrentAuthorization(User, MediaWiki\Extensions\OAuth\MWOAuthConsumer, string)
#3 /srv/mediawiki/php-1.34.0-wmf.13/extensions/OAuth/includes/frontend/specialpages/SpecialMWOAuth.php(106): MediaWiki\Extensions\OAuth\SpecialMWOAuth->handleAuthorizationForm(string, string, boolean)
#4 /srv/mediawiki/php-1.34.0-wmf.13/includes/specialpage/SpecialPage.php(571): MediaWiki\Extensions\OAuth\SpecialMWOAuth->execute(string)
#5 /srv/mediawiki/php-1.34.0-wmf.13/includes/specialpage/SpecialPageFactory.php(581): SpecialPage->run(string)
#6 /srv/mediawiki/php-1.34.0-wmf.13/includes/MediaWiki.php(288): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#7 /srv/mediawiki/php-1.34.0-wmf.13/includes/MediaWiki.php(884): MediaWiki->performRequest()
#8 /srv/mediawiki/php-1.34.0-wmf.13/includes/MediaWiki.php(515): MediaWiki->main()
#9 /srv/mediawiki/php-1.34.0-wmf.13/index.php(42): MediaWiki->run()
#10 /srv/mediawiki/w/index.php(3): require(string)

Impact

(To be determined.)

Notes

Started with the deployment of 1.34.0-wmf.13, and not seen in weeks prior.

Event Timeline

Krinkle created this task.Jul 10 2019, 5:29 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 10 2019, 5:29 PM
greg added a subscriber: greg.Jul 10 2019, 5:45 PM

@Krinkle I can't see the errors in logstash (maybe just because we're only on group0 with wmf.13?), how frequent was this error when you saw it?

Tgr added a subscriber: Tgr.Jul 10 2019, 6:12 PM

rMWdd6b94024c53: Re-apply: Factors out permissions check from User into PermissionManager service would be the obvious culprit as it added a __get() magic method to User, but the timing doesn't match (it was merged 12 days ago).

Tgr added a comment.Jul 10 2019, 6:21 PM

Oh wait, no train last week due to July 4. It does match then.

I guess this is a PHP bug of sorts? If $o has a magic getter/setter and isset( $o->foo ) is false, $o->foo['bar'] = 'baz' should invoke the setter, not the getter.

Tgr added a comment.Jul 10 2019, 6:35 PM

I guess this is a PHP bug of sorts?

PHP devs seem to disagree, although it's hard to see from the comments why: #42030

Change 521928 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] User: support setting custom fields + array autocreation in non-existent field

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

Change 521930 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/OAuth@master] Do not rely on array autocreation for custom User properties

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

Change 521931 had a related patch set uploaded (by Krinkle; owner: Gergő Tisza):
[mediawiki/extensions/OAuth@wmf/1.34.0-wmf.13] Do not rely on array autocreation for custom User properties

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

Tgr added a comment.Jul 10 2019, 7:38 PM
public function __set( $name, $value ) {
	// A shortcut for $mRights deprecation phase, only known legitimate use was for
	// testing purposes, other uses seem bad in principle

Breaking functionality without warning because it "seems bad in principle" is really not the way to do these things :(

This will break Special:MultiLock and several not-in-Wikimedia-production extensions.

Change 521930 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] Do not rely on array autocreation for custom User properties

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

Jdforrester-WMF triaged this task as Unbreak Now! priority.Jul 10 2019, 8:51 PM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJul 10 2019, 8:51 PM

Change 521928 merged by jenkins-bot:
[mediawiki/core@master] User: support setting custom fields + array autocreation in non-existent field

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

Change 521957 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@wmf/1.34.0-wmf.13] User: support setting custom fields + array autocreation in non-existent field

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

Change 521957 merged by jenkins-bot:
[mediawiki/core@wmf/1.34.0-wmf.13] User: support setting custom fields + array autocreation in non-existent field

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

Change 521931 merged by jenkins-bot:
[mediawiki/extensions/OAuth@wmf/1.34.0-wmf.13] Do not rely on array autocreation for custom User properties

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

Mentioned in SAL (#wikimedia-operations) [2019-07-10T22:57:40Z] <jforrester@deploy1001> Synchronized php-1.34.0-wmf.13/includes/user/User.php: T227688 User: support setting custom fields + array autocreation in non-existent field (duration: 00m 58s)

Mentioned in SAL (#wikimedia-operations) [2019-07-10T23:02:24Z] <jforrester@deploy1001> Synchronized php-1.34.0-wmf.13/extensions/OAuth/includes/backend/MWOAuthUtils.php: T227688 OAuth: Do not rely on array autocreation for custom User properties; re-try (duration: 00m 58s)

Jdforrester-WMF closed this task as Resolved.Jul 10 2019, 11:06 PM
Jdforrester-WMF removed a project: Patch-For-Review.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Deployed.

Tgr added a comment.EditedJul 11 2019, 10:27 AM

Follow-ups left to do:

Anomie added a subscriber: Anomie.Jul 11 2019, 6:20 PM

I guess this is a PHP bug of sorts?

PHP devs seem to disagree, although it's hard to see from the comments why: #42030

As far as I can tell, to "fix" this they'd have to make

$obj->foo['bar'] = 1;

work as if it were written something like

$tmp = $obj->__get( 'foo' );
$tmp['bar'] = 1;
$obj->__set( 'foo', $tmp );

and for whatever reason they don't want to do that. It might be as simple as that reliably detecting the situation would be too complicated given the even lower level operations involved. Instead they have a heuristic to give a warning when it looks like an array returned by __get() is being modified.

Unfortunately the PHP devs involved there don't seem to have liked linking back to wherever they gave the reason (extreme example: #39725), so it's very difficult to find. I was unable to find it. Although while I was looking, I found #41641 which was sadly amusing as an example of miscommunication.

Tgr added a comment.Jul 11 2019, 8:09 PM

PHP handles

$obj->foo['bar'] = 1;

as

$tmp = $obj->__get( 'foo' );
$tmp['bar'] = 1;

which works fine if foo exists and __get is declared to return references . (Which IIRC was somehow problematic in older PHP and has been fixed recently, but I can't find any docs about that so maybe I'm confusing things.) The issue is when foo doesn't exist, there is no good way to handle the initial $obj->__get( 'foo' ) call. The nice way would be if PHP handled it like this instead:

if ( !$obj->__isset( 'foo' ) ) {
    $obj->__set( 'foo', [] );
}
$tmp = $obj->__get( 'foo' );
$tmp['bar'] = 1;

which is IMO not that unreasonable as native properties behav ein a similar way.