Page MenuHomePhabricator
Create Task
Maniphest T270228

"Error: Unsupported operand types" from ParserOptionsTest::testMatches on php74
Closed, ResolvedPublic
Actions

Description

https://travis-ci.com/github/wikimedia/mediawiki/builds
https://travis-ci.com/github/wikimedia/mediawiki/jobs/460845768

There was 1 error:

1) ParserOptionsTest::testMatches

Error: Unsupported operand types

/home/travis/build/wikimedia/mediawiki/tests/phpunit/includes/parser/ParserOptionsTest.php:283
ParserOptionsTest.php
		$classWrapper = TestingAccessWrapper::newFromClass( ParserOptions::class );
…
		$classWrapper->defaults += [ __METHOD__ => null ]; # BOOM

The last commit to this code path was:

The last commit on which Travis CI passed on php74:

Details

SubjectRepoBranchLines +/-
Mark ParserOptionsTests skipped on PHP 7.4.0-7.4.8mediawiki/coreREL1_35+7 -1
Make ParserOptionsTest work on PHP<7.4.9 with opcache enabledmediawiki/coreREL1_35+112 -64
Make ParserOptionsTest work on PHP<7.4.9 with opcache enabledmediawiki/coremaster+111 -65
PHPVersionCheck: Complain about known-bad versions above minimummediawiki/coreREL1_35+51 -10
PHPVersionCheck: Complain about known-bad versions above minimummediawiki/coreREL1_31+716 -1
PHPVersionCheck: Complain about known-bad versions above minimummediawiki/coremaster+55 -11
ParserOptions::matches shouldn't try to stringify closuresmediawiki/coremaster+7 -16
Customize query in gerrit

Related Objects

Event Timeline

Krinkle created this task.Dec 16 2020, 3:46 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 16 2020, 3:46 AM
• Pchelolo edited projects, added Platform Team Workboards (Green); removed Platform Engineering.Dec 16 2020, 2:56 PM
• Pchelolo added a comment.Dec 16 2020, 3:39 PM

Interesting. Locally with php 7.4.7 and vanilla MediaWiki master the tests pass for me. The line that crashes is trying to add an array to TestingAccessWrapper defaults, which corresponds to ParserOptions::$defaults which is lazily initialized from any ParserOptions constructor.

• Pchelolo added a subscriber: Reedy.Dec 16 2020, 3:59 PM

I would argue that the reason for this could probably be https://gerrit.wikimedia.org/r/c/mediawiki/core/+/643807 - update of testing-access-wrapper to 2.0

Reedy added a subscriber: MaxSem.EditedDec 16 2020, 4:21 PM
In T270228#6695748, @Pchelolo wrote:

I would argue that the reason for this could probably be https://gerrit.wikimedia.org/r/c/mediawiki/core/+/643807 - update of testing-access-wrapper to 2.0

Pinging T268846: Update MediaWiki to a new wikimedia/testing-access-wrapper release

https://github.com/wikimedia/testing-access-wrapper/compare/1.0.0...2.0.0

As it's a private static (why is it being called -> rather than ::? magic __get()?).. I wonder if T248887: Make testing-access-wrapper compatible with PHP8 and rTAWR553bfb7189d3: PHP8 compatibility are to blame/involved. The actual code changes otherwise are minimal, or almost certainly not related

• Pchelolo edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Team Workboards (Green).Dec 16 2020, 5:27 PM

I'm going to move this to clinic duty, it doesn't seem like immediate fallout from the ParserCache work.

gerritbot added a comment.Dec 16 2020, 6:07 PM

Change 649947 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] ParserOptions::matches shouldn't try to stringify closures

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

gerritbot added a project: Patch-For-Review.Dec 16 2020, 6:07 PM
gerritbot added a comment.Dec 16 2020, 9:14 PM

Change 649947 abandoned by Ppchelko:
[mediawiki/core@master] ParserOptions::matches shouldn't try to stringify closures

Reason:
scratch this. I misunderstood a thing or two

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

Krinkle added a comment.Dec 16 2020, 10:04 PM
In T270228#6695792, @Reedy wrote:
In T270228#6695748, @Pchelolo wrote:

I would argue that the reason for this could probably be https://gerrit.wikimedia.org/r/c/mediawiki/core/+/643807 - update of testing-access-wrapper to 2.0

Pinging T268846: Update MediaWiki to a new wikimedia/testing-access-wrapper release

https://github.com/wikimedia/testing-access-wrapper/compare/1.0.0...2.0.0

As it's a private static (why is it being called -> rather than ::? magic __get()?).. I wonder if T248887 and 553bfb7189d3 are to blame/involved. […]

Good call. If there's difference between the version used in composer vs vendor, that could explain it since we're testing php74 with vendor, but otherwise use composer elsewhere (e.g. in Travis CI, and local dev for me).

• Pchelolo added a comment.Dec 16 2020, 10:35 PM

I think I found the root cause: https://bugs.php.net/bug.php?id=79487

Reedy added a comment.Dec 16 2020, 10:42 PM
In T270228#6697186, @Pchelolo wrote:

I think I found the root cause: https://bugs.php.net/bug.php?id=79487

And the fix for that is in 7.4.9, from 2020-08-06... https://www.php.net/ChangeLog-7.php#7.4.9

I guess we need to bare that in mind as a potentially relevant PHP version when we bump to 7.4 as a minimum...

gerritbot added a comment.Dec 16 2020, 11:16 PM

Change 649947 restored by Ppchelko:
[mediawiki/core@master] ParserOptions::matches shouldn't try to stringify closures

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

gerritbot added a comment.Dec 16 2020, 11:27 PM

Change 649947 abandoned by Ppchelko:
[mediawiki/core@master] ParserOptions::matches shouldn't try to stringify closures

Reason:

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

• Pchelolo added a comment.Dec 16 2020, 11:29 PM

So, the overall problem seems to be:

  • In wikimedia/testing-access-wrapper 2.0 a fix for php-8 compatibility was made 553bfb7189d3 which made us use ReflectionClass::getStaticProperties
  • In php 7.4 that method was broken, so now with Opcache enabled, changes made to static properties are no longer reflected in the result of ReflectionClass::getStaticProperties.
  • The ParserOptions::$defaults property initialized as null and then lazily changed to an array. TestingAccessWrapper though still returns null, so attempt to merge it with an array crashes.

I went into fixing the test, there's a way to verify what the test is trying to verify without testing access wrapper, but then I realized that the method being tested appears to be unused anywhere. Maybe we should just deprecate it and drop the test?

• AMooney triaged this task as Medium priority.Dec 22 2020, 9:00 PM
• AMooney moved this task from Inbox to Later on the Platform Team Workboards (Clinic Duty Team) board.
Krinkle renamed this task from ParserOptionsTest::testMatches fails on plain php74 to "Error: Unsupported operand types" from ParserOptionsTest::testMatches on php74.Jan 29 2021, 1:39 AM
Krinkle added a comment.Jan 29 2021, 1:43 AM
In T270228#6697187, @Reedy wrote:
In T270228#6697186, @Pchelolo wrote:

I think I found the root cause: https://bugs.php.net/bug.php?id=79487

And the fix for that is in 7.4.9, from 2020-08-06... https://www.php.net/ChangeLog-7.php#7.4.9

I guess we need to bare that in mind as a potentially relevant PHP version when we bump to 7.4 as a minimum...

We may want to change the way minimum versions are encoded. If we assume a conservative support policy (as we have had so far), then that means the only people benefiting from our "this PHP version is broken" detection are people lagging on the oldest supported PHP version at the time of installation. That seems a rather limited value.

The time most wikis (by quantity) come across PHP 7.4 is presumably between now and some time before (not after) we raise the minimum version to be at or above 7.4.

Krinkle added a project: MW-1.36-release.Jan 29 2021, 1:44 AM
Jdforrester-WMF subscribed.Jan 29 2021, 5:22 PM

Expanding PHPVersionCheck with a "known bad" deny list of versions of PHP wouldn't be too hard, or a significant overhead. Sure, let's do that.

gerritbot added a comment.Jan 29 2021, 6:23 PM

Change 659970 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] PHPVersionCheck: Complain about known-bad versions about minimum

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

gerritbot added a comment.Feb 25 2021, 3:55 PM

Change 659970 merged by jenkins-bot:
[mediawiki/core@master] PHPVersionCheck: Complain about known-bad versions above minimum

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

gerritbot added a comment.Feb 25 2021, 3:56 PM

Change 666700 had a related patch set uploaded (by Reedy; owner: Jforrester):
[mediawiki/core@REL1_35] PHPVersionCheck: Complain about known-bad versions above minimum

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.33; 2021-03-02).Feb 25 2021, 4:00 PM
gerritbot added a comment.Feb 25 2021, 11:20 PM

Change 667000 had a related patch set uploaded (by Reedy; owner: Jforrester):
[mediawiki/core@REL1_31] PHPVersionCheck: Complain about known-bad versions above minimum

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

gerritbot added a comment.Feb 25 2021, 11:22 PM

Change 667000 abandoned by Reedy:
[mediawiki/core@REL1_31] PHPVersionCheck: Complain about known-bad versions above minimum

Reason:

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

gerritbot added a comment.Feb 25 2021, 11:30 PM

Change 666700 merged by jenkins-bot:
[mediawiki/core@REL1_35] PHPVersionCheck: Complain about known-bad versions above minimum

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

Mateusz_Konieczny subscribed.Feb 25 2021, 11:53 PM

Is it deliberate that it was included in a minor release of Mediawiki suddenly broke down on certain PHP versions?

For example it seems that OSM Wiki suddenly went offline as result of that, see https://github.com/openstreetmap/operations/issues/511

Reedy added a comment.Feb 25 2021, 11:56 PM
In T270228#6863104, @Mateusz_Konieczny wrote:

Is it deliberate that it was included in a minor release of Mediawiki suddenly broke down on certain PHP versions?

For example it seems that OSM Wiki suddenly went offline as result of that, see https://github.com/openstreetmap/operations/issues/511

There's been no "minor release" yet. You're following a development/release branch.

ReleaseTaggerBot added a project: MW-1.35-notes.Feb 26 2021, 12:00 AM
Mateusz_Konieczny added a comment.EditedFeb 26 2021, 12:06 AM

"Well when we wrote that we understood that the branch only got updates as things were released." ( https://github.com/openstreetmap/operations/issues/511#issuecomment-786314181 )

Is there some sane way to use chef both (1) get only releases without random breakage (2) do not rely on human manually updating versions due to security issues?

Reedy added a comment.Feb 26 2021, 12:18 AM

That's never been the case, unfortunately. We use the branches at the place for staging for the next release, which allows us to run CI and such.

For example, as of writing, there's 98 commits staged since the 1.35.1 release; https://github.com/wikimedia/mediawiki/compare/1.35.1...REL1_35

There's no way we could deal with that many commits at the point of trying to shephered a new security (and maintenance) release.

However, you do bring up a couple of good points; and one that @Jdforrester-WMF mentioned to me on IRC... Maybe we should have a tag/branch pointer that points to the "latest released" version. So it points at 1.35.1 now, and when we release 1.35.2, it is moved to point there instead.

FWIW, we try and do a security/maintenance release once a quarter.

And/or the other option is having some sort of machine readable API (I don't think we have anything currently) where you can poll it and get the latest point release for a branch. Then be able to use that in your chef recipe to be git checkout'd

I'm guessing you're not expecting your wiki to automatically upgrade to 1.36 when that is released; ie that'd be a manual action?

Mateusz_Konieczny added a comment.EditedFeb 26 2021, 12:27 AM

Maybe we should have a tag/branch pointer that points to the "latest released" version.

As I understand, git tags are standard for that

from https://github.com/openstreetmap/operations/issues/511#issuecomment-786321627

As far as I can see there aren't any release tags in the repository

(...)

Actually the gerrit repos (which is what we use in chef) dont have tags, it's just the phabricator repos which do.

(do -> dont edit is mine based on context)
(...)

I guess I'll need to hack up something to parse git ls-remote and figure out the most recent tag and then sync to that.

I'm guessing you're not expecting your wiki to automatically upgrade to 1.36 when that is released; ie that'd be a manual action?

No idea. I am just confused person with some programming experience (without sysadmin experience) that is copying comments between two issue trackers.

And I go involved because I got "muahahahaha, wiki is down" screen in the middle of edit to https://wiki.openstreetmap.org/w/index.php?title=Talk:Key:amenity&action=history (literally in the middle, I used preview).

TomH72 subscribed.Feb 26 2021, 12:34 AM

No we're only trying to track point releases - upgrading to new major versions is done manually.

Mateusz_Konieczny added a comment.Feb 26 2021, 9:18 PM

Is 1.35.2 going to be released with this PHP restriction?

Would it be useful to create a separate issue requesting to avoid this?

Reedy added a comment.Feb 26 2021, 9:21 PM
In T270228#6865670, @Mateusz_Konieczny wrote:

Is 1.35.2 going to be released with this PHP restriction?

Probably, in a month or so.

Would it be useful to create a separate issue requesting to avoid this?

The proper task is T246594: Prevent use of known buggy versions of PHP (that are greater than the minimum supported PHP version) (7.4.0 – 7.4.8, and 7.3.0 - 7.3.18). We know certain versions of PHP are known buggy, so people shouldn't be using them...

Mateusz_Konieczny mentioned this in T246594: Prevent use of known buggy versions of PHP (that are greater than the minimum supported PHP version) (7.4.0 – 7.4.8, and 7.3.0 - 7.3.18).Feb 26 2021, 9:30 PM
tstarling subscribed.Mar 1 2021, 3:39 AM

PHP 7.4.3 is the version used by Ubuntu 20.04, that's going to be the current LTS of Ubuntu until 2022. Would it not be less disruptive to just remove the test? I don't really agree with TestingAccessWrapper in general -- you can always add internal public methods to help tests.

gerritbot added a comment.Mar 1 2021, 5:27 AM

Change 667426 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/core@master] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

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

tstarling added a comment.Mar 1 2021, 5:32 AM

I fixed the test so that it works on PHP 7.4.3 with -dopcache.enable_cli=1. I completely removed TestingAccessWrapper from that test, because it annoys me a lot. It seems like every time I change a private property in some common class, it turns out a test was depending on it. There's no point in having private variables if you can't change them without a caller survey.

MarkAHershberger subscribed.Mar 1 2021, 4:03 PM

Thanks for calling out the problem with Ubuntu 20.04, @tstarling. We just had someone come into the MediaWiki-Stakeholders-Group channel talking about this issue for 1.35 this morning, so this is a nice bit of serendipity.

gerritbot added a comment.Mar 2 2021, 8:21 PM

Change 667426 merged by jenkins-bot:
[mediawiki/core@master] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

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

gerritbot added a comment.Mar 2 2021, 8:36 PM

Change 667819 had a related patch set uploaded (by Reedy; owner: Tim Starling):
[mediawiki/core@REL1_35] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

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

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.34; 2021-03-09); removed MW-1.36-notes (1.36.0-wmf.33; 2021-03-02).Mar 2 2021, 9:00 PM
Reedy added a comment.Mar 10 2021, 6:06 AM
In T270228#6876240, @gerritbot wrote:

Change 667819 had a related patch set uploaded (by Reedy; owner: Tim Starling):
[mediawiki/core@REL1_35] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

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

It seems backporting this may result in needing quite a few supporting backports...

If the problem is just one test (or set of tests) on a specific range of PHP versions... For MW-1.35-release and the expected point release at the end of the month, would the simpler fix just be to mark the test skipped on the known broken versions of PHP, and remove the hard blocker?

gerritbot added a comment.Mar 10 2021, 6:12 AM

Change 670340 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@REL1_35] Mark ParserOptionsTests skipped on PHP 7.4.0-7.4.8

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

gerritbot added a comment.Mar 21 2021, 7:05 PM

Change 670340 abandoned by Jforrester:
[mediawiki/core@REL1_35] Mark ParserOptionsTests skipped on PHP 7.4.0-7.4.8

Reason:
We fixed this otherwise.

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

gerritbot added a comment.Mar 22 2021, 9:39 AM

Change 667819 abandoned by Reedy:
[mediawiki/core@REL1_35] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

Reason:

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

gerritbot added a comment.Mar 25 2021, 3:01 PM

Change 667819 restored by Jforrester:
[mediawiki/core@REL1_35] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

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

gerritbot added a comment.Mar 29 2021, 3:30 PM

Change 667819 abandoned by Reedy:
[mediawiki/core@REL1_35] Make ParserOptionsTest work on PHP<7.4.9 with opcache enabled

Reason:
Trying to get all the depandancies in to make this work isn't worth it

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

gerritbot added a comment.Mar 29 2021, 3:30 PM

Change 670340 restored by Reedy:
[mediawiki/core@REL1_35] Mark ParserOptionsTests skipped on PHP 7.4.0-7.4.8

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

gerritbot added a comment.Mar 29 2021, 8:28 PM

Change 670340 merged by jenkins-bot:
[mediawiki/core@REL1_35] Mark ParserOptionsTests skipped on PHP 7.4.0-7.4.8

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

Reedy closed this task as Resolved.Mar 29 2021, 8:29 PM
Reedy assigned this task to tstarling.
Reedy removed a project: Patch-For-Review.
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 · Credits