Linter: PHP Warning: in_array() expects parameter 2 to be array, null given
Closed, ResolvedPublicPRODUCTION ERROR
Actions

Assigned To

None

Authored By

	brennen
	Mar 2 2022, 7:59 PM

Description

Error

mwversion: 1.38.0-wmf.24
reqId: 78d2f7ce-3f02-4b05-b27e-9a7c47353360
Find reqId in Logstash

normalized_message

[{reqId}] {exception_url}   PHP Warning: in_array() expects parameter 2 to be array, null given

exception.trace

from /srv/mediawiki/php-1.38.0-wmf.24/extensions/Linter/includes/Hooks.php(151)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.38.0-wmf.24/extensions/Linter/includes/Hooks.php(151): in_array(string, NULL)
#2 /srv/mediawiki/php-1.38.0-wmf.24/includes/HookContainer/HookContainer.php(338): MediaWiki\Linter\Hooks::onRevisionFromEditComplete(WikiFilePage, MediaWiki\Revision\RevisionStoreRecord, integer, User, NULL)
#3 /srv/mediawiki/php-1.38.0-wmf.24/includes/HookContainer/HookContainer.php(137): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#4 /srv/mediawiki/php-1.38.0-wmf.24/includes/HookContainer/HookRunner.php(3184): MediaWiki\HookContainer\HookContainer->run(string, array)
#5 /srv/mediawiki/php-1.38.0-wmf.24/includes/filerepo/file/LocalFile.php(2055): MediaWiki\HookContainer\HookRunner->onRevisionFromEditComplete(WikiFilePage, MediaWiki\Revision\RevisionStoreRecord, integer, User, NULL)
#6 /srv/mediawiki/php-1.38.0-wmf.24/includes/filerepo/file/LocalFile.php(1824): LocalFile->recordUpload3(string, string, string, User, array, string, NULL, boolean, boolean)
#7 /srv/mediawiki/php-1.38.0-wmf.24/includes/upload/UploadBase.php(960): LocalFile->upload(string, string, string, integer, array, boolean, User, NULL)
#8 /srv/mediawiki/php-1.38.0-wmf.24/includes/api/ApiUpload.php(913): UploadBase->performUpload(string, string, boolean, User, NULL, NULL)
#9 /srv/mediawiki/php-1.38.0-wmf.24/includes/api/ApiUpload.php(167): ApiUpload->performUpload(array)
#10 /srv/mediawiki/php-1.38.0-wmf.24/includes/api/ApiUpload.php(124): ApiUpload->getContextResult()
#11 /srv/mediawiki/php-1.38.0-wmf.24/includes/api/ApiMain.php(1892): ApiUpload->execute()
#12 /srv/mediawiki/php-1.38.0-wmf.24/includes/api/ApiMain.php(870): ApiMain->executeAction()
#13 /srv/mediawiki/php-1.38.0-wmf.24/includes/api/ApiMain.php(841): ApiMain->executeActionWithErrorHandling()
#14 /srv/mediawiki/php-1.38.0-wmf.24/api.php(90): ApiMain->execute()
#15 /srv/mediawiki/php-1.38.0-wmf.24/api.php(45): wfApiMain()
#16 /srv/mediawiki/w/api.php(3): require(string)
#17 {main}

Impact

Unclear.

Notes

~20 instances since deploy of 1.38.0-wmf.24 (T300200) to group1; mostly on commons.

Details

Request URL: https://commons.wikimedia.org/w/api.php?action=upload&maxlag=*&format=*&utf8=*

Subject	Repo	Branch	Lines +/-
Hooks.php: Check for non-array $tags	mediawiki/extensions/Linter	master	+6 -0
Try to ensure tags are arrays	mediawiki/core	master	+3 -3
ApiUpload: Pass empty tags as empty array instead of null	mediawiki/core	master	+2 -2
Hooks.php: Check for non-array $tags	mediawiki/extensions/Linter	wmf/1.38.0-wmf.24	+6 -0

Customize query in gerrit

Related Objects

Mentioned Here: T300200: 1.38.0-wmf.24 deployment blockers

Event Timeline

brennen created this task.Mar 2 2022, 7:59 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 2 2022, 7:59 PM

brennen moved this task from Backlog to Logs/Train on the User-brennen board.Mar 2 2022, 8:00 PM

The trace is attributed to a line of code in the Linter extension, which is maintained by the newly named "Content Transform" team. Afaik they have yet to update their workflow to include a Phab tag (including no alias under an existing tag). Tagging Parsoid as such, which I happen to know is a workboard this team uses for most their work.

The class within the Linter extension tha is called "Hooks" and makes use of the core Hooks system, however I do not see any indication that is is likely an issue there. If it was filed there on purpose instead of Linter, feel free to undo. (I'm assuming this not to be the case since it wasn't filed there by a MW dev, and in the interest of effeciency am boldly re-tagging it as such).

Alternatively, it is possible that the array in question was broken by the producer of the hook. The trace indicates that the hook callback came from the Upload feature. So if this is not an issue in Linter MediaWiki-extensions-Linter than it is likely a problem for Upload MediaWiki-Uploading.

If it was filed there on purpose instead of Linter, feel free to undo. (I'm assuming this not to be the case since it wasn't filed there by a MW dev, and in the interest of effeciency am boldly re-tagging it as such).

This seems correct; I was taking a wild guess.

I have not yet rolled back, but was contemplating it. There are currently ~40 instances. Any guesses as to likelihood of data loss?

Lint errors will get repopulated. So, not a data loss issue in Linter itself. But, this may be a problem in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Linter/+/766195/1/includes/Hooks.php#129 ... Pinged @Arlolra about it.

Actually not ... I had a stale checkout and line 151 points to ( in_array( "mw-contentmodelchange", $tags ).

Change 767582 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/extensions/Linter@master] Hooks.php: Check for null $tags

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

gerritbot added a project: Patch-For-Review.Mar 2 2022, 8:42 PM

In T302918#7748693, @Krinkle wrote:

...
Alternatively, it is possible that the array in question was broken by the producer of the hook. The trace indicates that the hook callback came from the Upload feature. So if this is not an issue in Linter MediaWiki-extensions-Linter than it is likely a problem for Upload MediaWiki-Uploading.

I uploaded a patch above with a defensive null check, but yes, not sure if null (vs []) is valid there.

And, not sure how it got past line 150 which also as a in_array( .. $tags) check.

And, not sure how it got past line 150 which also as a in_array( .. $tags) check.

Looking closer at logs, there are ~~errors~~ warnings for both line 150 and 151.

Once this patch merges, feel free to backport @brennen.

And as @Krinkle hinted at and as @Arlolra, @Sbailey and I have all independently investigated and found that callers of the hook don't comply with the advertised hook signature and without strict type checks, that isn't enforced. @Arlolra is recommending that we should fix those callers. But, without strict type checks, we won't be able to prevent future non-compliant callers from creeping in without strict code review or other means.

Change 767582 merged by jenkins-bot:

[mediawiki/extensions/Linter@master] Hooks.php: Check for non-array $tags

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

Once this patch merges, feel free to backport @brennen.

Will do, thanks all.

Change 767104 had a related patch set uploaded (by Brennen Bearnes; author: Subramanya Sastry):

[mediawiki/extensions/Linter@wmf/1.38.0-wmf.24] Hooks.php: Check for non-array $tags

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

Change 767104 merged by jenkins-bot:

[mediawiki/extensions/Linter@wmf/1.38.0-wmf.24] Hooks.php: Check for non-array $tags

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

Mentioned in SAL (#wikimedia-operations) [2022-03-02T21:59:44Z] <brennen@deploy1002> Synchronized php-1.38.0-wmf.24/extensions/Linter/includes/Hooks.php: Backport: [[gerrit:767104|Hooks.php: Check for non-array $tags (T302918)]] (duration: 00m 50s)

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.24; 2022-02-28).Mar 2 2022, 10:00 PM

Maintenance_bot removed a project: Patch-For-Review.Mar 2 2022, 10:10 PM

In T302918#7748957, @ssastry wrote:

[…] and found that callers of the hook don't comply with the advertised hook signature, and […]

The above point rings untrue to me because the onRevisionFromEditComplete hook is in fact consistently documented everywhere as string[] and has been for a while, as evidenced by its copies in all users of the hook also typing it that way. (e.g. hook class doc, and mediawiki.org page).

If this was typed as |null, then users would have likely complied with this, plus Phan would have likely caught it as well.

I'm aware that the WikiPage.php code that fires this hook takes a nullable $tags parameter. But, this seems like an internal implementation detail we shouldn't expect users to be aware of. It also seems odd to me that we pass that down to the hook since this has afaik no special meaning that is different from an empty array. It is passed as-is to addTags which also effectively treats it the same way as an empty array.

The above is a correction and a minor suggestion. I don't mind going in this direction for other reasons if that's what you think is best.

The trace above is saying that a null from $this->mParams['tags'], is making its way to the hook, which is not a path through WikiPage,
https://github.com/wikimedia/mediawiki/blob/master/includes/api/ApiUpload.php#L912

The same can happen through another action api route, though this one does go through WikiPage,
https://github.com/wikimedia/mediawiki/blob/master/includes/api/ApiProtect.php#L146

The docs for WikiPage::doUpdateRestrictions even seem more lenient @param string|string[]|null though I don't think anything is passing it a string,
https://github.com/wikimedia/mediawiki/blob/master/includes/page/WikiPage.php#L2223