Page MenuHomePhabricator

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

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:

Event Timeline

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.

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

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

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

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

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

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

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

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

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

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

Reason:

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

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

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.

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.

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

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

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

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

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

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

Reason:

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

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

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

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

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.

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

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?

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

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

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?

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

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.

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

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.

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.

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

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

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?

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

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

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

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

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

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

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 assigned this task to tstarling.
Reedy removed a project: Patch-For-Review.