@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?

Got it: https://logstash.wikimedia.org/goto/d11569bd468085431327400e8ff9db1d

4 of them on mw.org today.

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).

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.

In T227688#5322066, @Tgr wrote:

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

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

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)

Follow-ups left to do:

• document the deprecation / breakage. Done in https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/522054
• re-add the ability to temporarily change user rights for the current request, or do something else about the bot mode in Special:MultiLock or remove the feature and notify stewards - T227772: Fix or remove capability to override user rights for the current request

In T227688#5322123, @Tgr wrote:

In T227688#5322066, @Tgr wrote:

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.

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.