Page MenuHomePhabricator

Confirm DatabaseMysqlBase::numRows error suppression works as intended (non-object in DatabaseMysqli::mysqlNumRows)
Closed, DuplicatePublic

Description

Steps to reproduce:

  • go to the beta deploy wiki, get a PHP shell
  • run User::newSystemUser('<non-existent user>')

Will yield PHP Notice: Trying to get property of non-object in /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/DatabaseMysqli.php on line 233
The full trace is

 1:  Psy\Shell->handleError() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/DatabaseMysqli.php:233
 2:  Wikimedia\Rdbms\DatabaseMysqli->mysqlNumRows() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/DatabaseMysqlBase.php:362
 3:  Wikimedia\Rdbms\DatabaseMysqlBase->numRows() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/Database.php:1257
 4:  Wikimedia\Rdbms\Database->doProfiledQuery() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/Database.php:1155
 5:  Wikimedia\Rdbms\Database->query() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/Database.php:3893
 6:  Wikimedia\Rdbms\Database->doCommit() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/Database.php:3864
 7:  Wikimedia\Rdbms\Database->commit() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/Database.php:3664
 8:  Wikimedia\Rdbms\Database->endAtomic() at /srv/mediawiki-staging/php-master/includes/libs/rdbms/database/Database.php:3752
 9:  Wikimedia\Rdbms\Database->doAtomicSection() at /srv/mediawiki-staging/php-master/includes/user/User.php:4322
10:  User::createNew() at /srv/mediawiki-staging/php-master/includes/user/User.php:848
11:  User::newSystemUser() at eval()'d code:2

Event Timeline

Apparently DatabaseMysqlBase::numRows() expects $numRows to be a resource (or a ResultWrapper which is then turned into a resource) but DatabaseMysqli::mysqlNumRows() expects a mysqli_result object. The error caused by this is normally not shown because the call is wrapped in suppressWarnings. But the same is true for most DatabaseMysqlBase/DatabaseMysqli method pairs and they are probably not all broken or there would be more fallout, so some of the type documentation must be wrong.

I don't recall why mysqlNumRows() has its error suppressed, but possibly to suppress exactly this error as false-negative. I see it in production logs with suppressed: true only, so it seems fine.

Do you find any resulting behaviour that seems incorrect?

If we do look into this, it would help if you could report the actual value at that point in the code. The typical example of a non-object is null, but that seems unlikely here given that it is only called if the ternary returned true in DatabaseMysqlBase::numRows before calling DatabaseMysqli::mysqlNumRows. I'm curious what value that might be and whether that is indeed intentional or perhaps the result of an actual bug being suppressed.

Krinkle renamed this task from mysqlNumRows: Trying to get property of non-object in DatabaseMysqli.php to Confirm that DatabaseMysqlBase::numRows error suppression is working as intended (property of non-object in DatabaseMysqli::mysqlNumRows).Sep 17 2018, 3:54 PM
Krinkle renamed this task from Confirm that DatabaseMysqlBase::numRows error suppression is working as intended (property of non-object in DatabaseMysqli::mysqlNumRows) to Confirm DatabaseMysqlBase::numRows error suppression works as intended (non-object in DatabaseMysqli::mysqlNumRows).
Krinkle moved this task from Untriaged to Rdbms library on the Wikimedia-Rdbms board.
Krinkle changed Risk Rating from N/A to default.
Krinkle added a project: Performance-Team.