Page MenuHomePhabricator

PHP Warning: preg_match() expects parameter 2 to be string, array given
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error
normalized_message
[{reqId}] {exception_url}   PHP Warning: preg_match() expects parameter 2 to be string, array given
exception.trace
from /srv/mediawiki/php-1.39.0-wmf.1/includes/libs/ParamValidator/TypeDef/IntegerDef.php(26)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.39.0-wmf.1/includes/libs/ParamValidator/TypeDef/IntegerDef.php(26): preg_match(string, array)
#2 /srv/mediawiki/php-1.39.0-wmf.1/includes/libs/ParamValidator/ParamValidator.php(597): Wikimedia\ParamValidator\TypeDef\IntegerDef->validate(string, array, array, array)
#3 /srv/mediawiki/php-1.39.0-wmf.1/includes/libs/ParamValidator/ParamValidator.php(551): Wikimedia\ParamValidator\ParamValidator->validateValue(string, array, array, array)
#4 /srv/mediawiki/php-1.39.0-wmf.1/includes/Rest/Validator/Validator.php(108): Wikimedia\ParamValidator\ParamValidator->getValue(string, array, array)
#5 /srv/mediawiki/php-1.39.0-wmf.1/includes/Rest/Handler.php(184): MediaWiki\Rest\Validator\Validator->validateParams(array)
#6 /srv/mediawiki/php-1.39.0-wmf.1/includes/Rest/Router.php(405): MediaWiki\Rest\Handler->validate(MediaWiki\Rest\Validator\Validator)
#7 /srv/mediawiki/php-1.39.0-wmf.1/includes/Rest/Router.php(338): MediaWiki\Rest\Router->executeHandler(GrowthExperiments\Rest\Handler\MenteesHandler)
#8 /srv/mediawiki/php-1.39.0-wmf.1/includes/Rest/EntryPoint.php(167): MediaWiki\Rest\Router->execute(MediaWiki\Rest\RequestFromGlobals)
#9 /srv/mediawiki/php-1.39.0-wmf.1/includes/Rest/EntryPoint.php(132): MediaWiki\Rest\EntryPoint->execute()
#10 /srv/mediawiki/php-1.39.0-wmf.1/rest.php(31): MediaWiki\Rest\EntryPoint::main()
#11 /srv/mediawiki/w/rest.php(3): require(string)
#12 {main}
Impact

Moderate level of logspam during train deployment. Noticed during T300203: 🧪🚂 Trainsperiment Week: 1.39.0-wmf.1, 1.39.0-wmf.2, 1.39.0-wmf.3, 1.39.0-wmf.4 deployment blockers but not an immediate blocker.

Special:MentorDashboard's mentee overview module was not working, as it called the API endpoint with incorrect (and malformed) parameters, which didn't return the data required.

Notes

Seems to be related to bad client data. Noticed about the same time as T304349: PHP Notice: Undefined index: taskType - causes Argument 1 passed to GrowthExperiments\NewcomerTasks\NewcomerTasksChangeTagsManager::apply() must be of the type string, null given and could be related.

Event Timeline

dduvall lowered the priority of this task from Unbreak Now! to High.Mar 21 2022, 8:44 PM

The frequency of these errors is now pretty low and occurring in wmf.1 (currently deployed to all wikis) as well as wmf.2. Removing this as a blocker and lowering priority since promoting wmf.2 should not result in an increase in occurrence. If there is a more serious underlying cause here, we can reassess and decide whether or not to rollback.

The parameter limit%5BusersToShow%5D=10 gets handled as array and maybe the problem (Similiar to T301360)

But the rest of the query is also invalid, because offset=NaN is invalid.

Limit could use the LimitTypeDef or the client side sends better URI

I just looked through some of the implementations of TypeDef::validate(). They all failhard if they are called with an array in the $value parameter. They all expect a string, or at least somethign that can be coerced into a string.

The $value parameter comes from TypeDef::getValue() which is documented to return null|mixed. The default implementation delegates to Callbacks::getValue(), which is documented to return string|string[]|mixed.

It seems to me that all implementations of TypeDef::validate should check the parameter has the expected type and throw ValidationException if it doesnt.

LimitDef accepts an integer or "max", so that wouldn't help either.

In essence: we get an illegal value for that parameter, that needs to be fixed in the client (assuming it's not malicious). But MW should be fixed to fail gracefully. I'd consider this logspam. Doesn't need to block the train, I think.

Change 772508 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/core@master] Rest: Disallow array values in integer param

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

Change 772537 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] MenteeOverviewPresets.getUsersToShow: Fix typo

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

Change 772537 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] MenteeOverviewPresets.getUsersToShow: Fix typo

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

Change 772483 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@wmf/1.39.0-wmf.1] MenteeOverviewPresets.getUsersToShow: Fix typo

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

Change 772484 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@wmf/1.39.0-wmf.2] MenteeOverviewPresets.getUsersToShow: Fix typo

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

Change 772485 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@wmf/1.39.0-wmf.3] MenteeOverviewPresets.getUsersToShow: Fix typo

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

The client-side error was introduced with 4f72198 and should be fixed with e9d89a3. In addition to yielding a PHP Warning, it also breaks the mentor dashboard (as the API errors out instead of providing expected data). I'm backporting the fix now, so hopefully the errors should cease.

Change 772484 merged by Urbanecm:

[mediawiki/extensions/GrowthExperiments@wmf/1.39.0-wmf.2] MenteeOverviewPresets.getUsersToShow: Fix typo

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

Change 772485 merged by Urbanecm:

[mediawiki/extensions/GrowthExperiments@wmf/1.39.0-wmf.3] MenteeOverviewPresets.getUsersToShow: Fix typo

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

Mentioned in SAL (#wikimedia-operations) [2022-03-22T07:57:03Z] <urbanecm@deploy1002> Synchronized php-1.39.0-wmf.2/extensions/GrowthExperiments/modules/ext.growthExperiments.MentorDashboard/MenteeOverview/MenteeOverviewPresets.js: 84877bd: MenteeOverviewPresets.getUsersToShow: Fix typo (T304353) (duration: 00m 49s)

Urbanecm_WMF moved this task from Incoming to QA on the Growth-Team (Current Sprint) board.

This should now be resolved from Growth's POV, as the API client was fixed. A decision still needs to be made whether or not to proceed with the core patch as well.

Change 772483 abandoned by Urbanecm:

[mediawiki/extensions/GrowthExperiments@wmf/1.39.0-wmf.1] MenteeOverviewPresets.getUsersToShow: Fix typo

Reason:

probably not necessary

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

Urbanecm_WMF raised the priority of this task from High to Needs Triage.Mar 23 2022, 1:29 PM

I don't think this is High prio anymore, as it's not "logspam that's currently happening", but a chance to get logspam thanks to clients sending malformed requests.

Change 773339 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] ParamValidator: Handle array url parameters on validation

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

Change 772508 merged by jenkins-bot:

[mediawiki/core@master] ParamValidator: Disallow array values in integer param

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

Change 773339 abandoned by Umherirrender:

[mediawiki/core@master] ParamValidator: Handle array url parameters on validation

Reason:

outdated

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