Page MenuHomePhabricator

Installation of CheckUser barfs when run on a brand new mediawiki installation
Closed, ResolvedPublicBUG REPORT

Description

Steps to Reproduce:

Install a new wiki with the CheckUser extension then run maintenance/update.php to do the setup.

Actual Results:

Starting poulation of cu_changes with recentchanges rc_id from 1 to 100
...migrating rc_id from 1 to 100
STDERR: [3f2156535a55003e0b1889b8] [no req]   Wikimedia\Rdbms\DBQueryError from line 1587 of /srv/wiki.openstreetmap.org/w/includes/libs/rdbms/database/Database.php: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? 
Query: INSERT  INTO `cu_changes` (cuc_timestamp,cuc_user,cuc_user_text,cuc_namespace,cuc_title,cuc_comment,cuc_minor,cuc_page_id,cuc_this_oldid,cuc_last_oldid,cuc_type,cuc_ip,cuc_ip_hex) VALUES ('20200204191942',NULL,'MediaWiki default','0','Main_Page','','0','1','1','0','1','127.0.0.1','7F000001')
Function: PopulateCheckUserTable::doDBUpdates
Error: 1048 Column 'cuc_user' cannot be null (localhost)
       
Backtrace:
#0 /srv/wiki.openstreetmap.org/w/includes/libs/rdbms/database/Database.php(1556): Wikimedia\Rdbms\Database->getQueryExceptionAndLog(string, integer, string, string)
#1 /srv/wiki.openstreetmap.org/w/includes/libs/rdbms/database/Database.php(1274): Wikimedia\Rdbms\Database->reportQueryError(string, integer, string, string, boolean)
#2 /srv/wiki.openstreetmap.org/w/includes/libs/rdbms/database/Database.php(2149): Wikimedia\Rdbms\Database->query(string, string)
#3 /srv/wiki.openstreetmap.org/w/extensions/CheckUser/maintenance/populateCheckUserTable.php(98): Wikimedia\Rdbms\Database->insert(string, array, string)
#4 /srv/wiki.openstreetmap.org/w/maintenance/Maintenance.php(1719): PopulateCheckUserTable->doDBUpdates()
#5 /srv/wiki.openstreetmap.org/w/maintenance/update.php(215): LoggedUpdateMaintenance->execute()
#6 /srv/wiki.openstreetmap.org/w/maintenance/doMaintenance.php(96): UpdateMediaWiki->execute()
#7 /srv/wiki.openstreetmap.org/w/maintenance/update.php(275): require_once(string)
#8 {main}
---- End output of php maintenance/update.php --quick ----
Ran php maintenance/update.php --quick returned 255

Expected Results:

Installation succeeds.

Additional Information:

Looking at the database there is only one change present:

mysql> select rc_id, rc_user from recentchanges;
+-------+---------+
| rc_id | rc_user |
+-------+---------+
|     1 |       0 |
+-------+---------+
1 row in set (0.00 sec)

That change is mode by user 0 however, which doesn't seem to exist:

mysql> select user_id, user_name from user;
+---------+--------------+
| user_id | user_name    |
+---------+--------------+
|       2 | Abuse filter |
|       1 | Admin        |
+---------+--------------+
2 rows in set (0.00 sec)

In turn that seems to lead to trying to insert null into the cu_changes table.

Event Timeline

What version of MediaWiki/the extension?

'MediaWiki default' is in $wgReservedUsernames, it's not a physical user account

User 0 is usually "anon"/ip users, but seemingly we use it for reserved/system accounts too...

This is 1.33, specifically it is tracking the REL1_33 git branch for both the main code and the extension.

I’ve tested this patch and verified that it solves our immediate NULL insert problem:

https://github.com/migurski/extension-checkuser/commit/34c693295b9092b5954562eb68ead92d995ab7d5

I’m unsure what other effects this might have.

@MMigurski: Hi, if you would like to propose your code changes for inclusion, then you are very welcome to use developer access to submit them as a Git branch directly into Gerrit which allows review and providing feedback.

Thanks @Aklapper, I started the process and ended up with a change here: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CheckUser/+/572266/

This is on the master branch, but it would also need to be backported to the REL1_33 branch (and others?). I’m unsure how to represent version backports in Gerrit.

It seems like the next step is on your end, is that right?

Change 572266 had a related patch set uploaded (by Aklapper; owner: Michal Migurski):
[mediawiki/extensions/CheckUser@master] Added rc_user null check to prevent failing inserts

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

Change 572271 had a related patch set uploaded (by Aklapper; owner: Michal Migurski):
[mediawiki/extensions/CheckUser@REL1_33] Added rc_user null check to prevent failing inserts

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

I fixed the two changes in response to PHP quibbles, and both seem happy now.

Hi @Aklapper, just pinging the thread here to see if you’re able to check on this change. Thanks for your help getting this merged!

just pinging the thread here to see if you’re able to check on this change.

Hi, I cannot as I lack knowledge. According to https://www.mediawiki.org/wiki/Developers/Maintainers the code stewards for CheckUser is the Platform Engineering, hence adding them here.

Hi @Anomie, I’m curious to see if there’s been any progress on this change. We’re blocked on a pull request to OSM’s Chef infrastructure repo, and this would allow us to introduce testing for our wiki installation. Thanks!

Current status is that your patch has comments in code review that you need to address.

"Addressing" the comments can range from making the requested changes, to making a different change that you think address the identified issues in a better way, to attempting to convince the reviewers that the issues pointed out aren't actually problems.

Thanks for pointing that out; I apologize that I’m unfamiliar with Gerrit and did not see your suggested edits. I’ll test these on our side and update.

I’ve completed a test of your proposed change and updated Gerrit with two new commits.

Hi @Anomie, I think that both changes are up to date with your proposed modification. Looking forward to seeing these in Mediawiki!

Hi again @Anomie, just checking in on the status of this update. Thanks for all your help so far!

Hi again @Anomie, just checking in on the status of this update. Thanks for all your help so far!

He's been out this week

You don't need to keep pinging him, he will no doubt review it when he's back working

Anomie assigned this task to MMigurski.

Sorry for the delay, internal politics prevented me from reviewing for a few weeks. Thanks for the patch!

Thank you so much, Brad! I understand and I really appreciate your help in getting this integrated.