Page MenuHomePhabricator

FlaggedRevs: reviewAllPages.php CommentStore.php: $row does not contain fields needed for comment rev_comment
Open, Needs TriagePublic

Description

Happening in a new mediawiki installation where database was restored using maintenance script importDump.php. All pages are now unchecked. To avoid having to check 100's of pages. I wanted to use the following maintenance script.

php reviewAllPages.php --username Patrick

issue:

Auto-reviewing all current page versions...
Reviewer username: Patrick
...doing page_id from 2 to 101
InvalidArgumentException from line 302 of /var/www/w/includes/CommentStore.php: $row does not contain fields needed for comment rev_comment
#0 /var/www/w/includes/CommentStore.php(403): CommentStore->getCommentInternal(Object(Wikimedia\Rdbms\DBConnRef), 'rev_comment', Array, true)
#1 /var/www/w/includes/Revision/RevisionStore.php(1529): CommentStore->getCommentLegacy(Object(Wikimedia\Rdbms\DBConnRef), 'rev_comment', Object(stdClass), true)
#2 /var/www/w/includes/Revision/RevisionStore.php(1387): MediaWiki\Revision\RevisionStore->newRevisionFromRowAndSlots(Object(stdClass), NULL, 1, Object(Title), false)
#3 /var/www/w/extensions/FlaggedRevs/maintenance/reviewAllPages.php(79): MediaWiki\Revision\RevisionStore->newRevisionFromRow(Object(stdClass), 1)
#4 /var/www/w/extensions/FlaggedRevs/maintenance/reviewAllPages.php(31): ReviewAllPages->autoreview_current(Object(User))
#5 /var/www/w/maintenance/doMaintenance.php(107): ReviewAllPages->execute()
#6 /var/www/w/extensions/FlaggedRevs/maintenance/reviewAllPages.php(106): require_once('/var/www/w/main...')
#7 {main}

mediawiki version: 1.35.1

T252043 might be related.

Please advice.

Event Timeline

My quick guess is that the script should use RevisionStore::getQueryInfo, whereas it currently queries revision manually:

$res = $db->select( [ 'page', 'revision' ],
	'*',
	[ "page_id BETWEEN $blockStart AND $blockEnd",
		'page_namespace' => FlaggedRevs::getReviewNamespaces(),
		'rev_id = page_latest' ],
	__METHOD__
);

this doesn't account for comment & actor migrations and related new tables.

Since I'm pinged.
FlaggedRevs is unmaintained and my 4-year-long work to find a maintainer for it has been futile T185664: Code stewardship review: FlaggedRevs and its code quality is among the worst I've seen in my whole career (case in point). I'm sorry but I'm not gonna spend time on fixing anything on it unless it's causing major issues.

One thing I can suggest is to get rid of its overly complex parts. For example, you can simply delete the whole script and use the API instead.

I'm also getting this issue after installing a wiki's pages via importDump.php. Possible replication steps:

  1. Install MediaWiki 1.42 (version a398498e7058275d959a8ee8088f58b57cf76e19 in this case) and use the generated LocalSettings.php
    • This dump needs wfLoadExtensions(['TemplateStyles', 'Scribunto']); but I'm not sure that is relevant. No other extensions are loaded.
    • The only skin loaded is Vector.
  2. Download the pages XML from https://archive.org/download/wmau_officialwiki_2023-10-22
  3. Import it with $ php maintenance/importDump.php ~/tmp/officialwiki-pages_2023-10-22.xml.gz --no-updates
  4. Run $ php maintenance/rebuildtextindex.php and see the error:
Clearing searchindex table...Done
Rebuilding index fields for 2802 pages...
InvalidArgumentException from line 181 of mediawiki/includes/CommentStore/CommentStore.php: $row does not contain fields needed for comment rev_comment
#0 mediawiki/includes/CommentStore/CommentStore.php(277): MediaWiki\CommentStore\CommentStore->getCommentInternal()
#1 mediawiki/includes/Revision/RevisionStore.php(1779): MediaWiki\CommentStore\CommentStore->getCommentLegacy()
#2 mediawiki/includes/Revision/RevisionStore.php(1625): MediaWiki\Revision\RevisionStore->newRevisionFromRowAndSlots()
#3 mediawiki/maintenance/rebuildtextindex.php(115): MediaWiki\Revision\RevisionStore->newRevisionFromRow()
#4 mediawiki/maintenance/rebuildtextindex.php(69): RebuildTextIndex->populateSearchIndex()
#5 mediawiki/maintenance/includes/MaintenanceRunner.php(703): RebuildTextIndex->execute()
#6 mediawiki/maintenance/doMaintenance.php(100): MediaWiki\Maintenance\MaintenanceRunner->run()
#7 mediawiki/maintenance/rebuildtextindex.php(167): require_once('...')
#8 {main}

Also getting the same issue after trying to use rebuildall.php with importDump.php with MediaWiki 1.41-beta releases. Is there any immediate hotfix that I can work with to get around the issue and use the rebuild script?

re @Samwilson no, not at all.

wfLoadExtensions( [
	// 'AbuseFilter',
	'AntiSpoof', 'Cite', 'CodeEditor',
	'CodeMirror', 'CSS', 'DisplayTitle',
	'Elastica', 'GeoData', 'Interwiki',
	'JsonConfig', 'Kartographer',
	'LabeledSectionTransclusion',
	// 'Loops',
	'NativeSvgHandler', 'NoTitle', 'Nuke',
	'ParserFunctions', 'PdfHandler',
	// 'ProtectSite',
	'Scribunto', 'SyntaxHighlight',
	'TabberNeue', 'TemplateData',
	'TemplateStyles', 'TemplateStylesExtender',
	'Translate', 'UniversalLanguageSelector',
	'UrlShortener',
	'VisualEditor', 'WikiEditor'
] );

This is all I have.

It sounds like perhaps this issue is unrelated to FlaggedRevs then. Should we change the task title and description? @adrelanos are you able to replicate the problem without FlaggedRevs enabled?

I tried commenting out all lines in my configuration that are loading extensions, and the issue persists. Looking back at T252043, the issue appeared since 1.35, so is it possible to be as T276782#6891983 outlined, having a issue with scripts not updated with the new database structure?

@adrelanos are you able to replicate the problem without FlaggedRevs enabled?

No. I don't have any similar issues without FlaggedRevisions. And I wouldn't know how to reproduce this issue without FlaggedRevisions. Because only by running php reviewAllPages.php --username Patrick this issue happened. And the reviewAllPages.php comes within the FlaggedRevisions folder.

Or do you mean I should test disable the FlaggedRevisions in mediawiki config and then run reviewAllPages.php?

@adrelanos are you able to replicate the problem without FlaggedRevs enabled?

No. I don't have any similar issues without FlaggedRevisions. And I wouldn't know how to reproduce this issue without FlaggedRevisions. Because only by running php reviewAllPages.php --username Patrick this issue happened. And the reviewAllPages.php comes within the FlaggedRevisions folder.

Or do you mean I should test disable the FlaggedRevisions in mediawiki config and then run reviewAllPages.php?

Our current case is that running rebuildall.php also breaks in the same way as they call the same part of CommentStore. Can you reproduce using rebuildall.php without FlaggedRevs? If you can, then this bug is not related to FlaggedRevs but to MediaWiki core itself, and we can rename the task and hand this to the right team to fix.

@adrelanos are you able to replicate the problem without FlaggedRevs enabled?

No. I don't have any similar issues without FlaggedRevisions. And I wouldn't know how to reproduce this issue without FlaggedRevisions. Because only by running php reviewAllPages.php --username Patrick this issue happened. And the reviewAllPages.php comes within the FlaggedRevisions folder.

Or do you mean I should test disable the FlaggedRevisions in mediawiki config and then run reviewAllPages.php?

Our current case is that running rebuildall.php also breaks in the same way as they call the same part of CommentStore. Can you reproduce using rebuildall.php without FlaggedRevs? If you can, then this bug is not related to FlaggedRevs but to MediaWiki core itself, and we can rename the task and hand this to the right team to fix.

I never had any issues running w/maintenance/rebuildall.php with or without FlaggedRevs enabled.

FlaggedRevs is enabled and before any MediaWiki upgrades on production servers, on the stage server rebuildall.phpis being run (among many other maintenance scripts) as kinda unit tests to see if there's any database, code issues. So I can confidently state that rebuildall.php always has been fine for me.

Thanks! The rebuild works now in my example. I guess a similar fix is needed for FlaggedRevs.