Page MenuHomePhabricator
Create Task
Maniphest T198342

Remove all usages of the 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo().
Closed, ResolvedPublic2 Estimated Story Points
Actions

Description

The 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo() causes a join of revision.rev_text_id against text.old_id. Since rev_text_id is no longer populated when disabling the legacy schema, all usages of it need to be eliminated first.

To find problematic calls to getQueryInfo, perhaps it helps to [[https://codesearch.wmflabs.org/extensions/?q=-%3Eold_&i=nope&files=&repos=|search for all references to fields that start with old_]].

DOD: This task should be considered done when any code using the 'text' flag has either been removed or is gated on the MCR schema migration stage, in core and all extensions deployed by Wikimedia, as well as extensions bundled with MediaWiki releases. Other extensions and external tools may be left to be updated later.

Extensions to fix:

  • Translate (T228675)
  • Flow (maintenance script)
  • ReplaceText (bundeled)

[] AbuseFilter

NOTE: fixes for this will probably coincide with fixes for T198343: Replace all calls to Revision::getRevisionText() and T198341: Remove all references to the rev_text_id and ar_text_id fields.

Details

SubjectRepoBranchLines +/-
Replace calls to Revision::getQueryInfo with the 'text' option set.mediawiki/coremaster+48 -21
Remove usages of 'text' flag in revision-related getQueryInfo() callsmediawiki/coremaster+84 -13
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 DeclineddchenT118706 Conduct heuristic evaluation of image upload and insert flow in VisualEditor
 OpenNoneT115858 Design improvements for mw.ForeignStructuredUpload.BookletLayout
 OpenNoneT115865 Insert image in content immediately after it's uploaded, skipping the "General settings" step
 DuplicateNoneT115864 Figure out if the description of the image can be used as the caption on-wiki
 OpenFeatureNoneT53032 When inserting an image, set its caption by default to be the Commons image description
 OpenBUG REPORTNoneT12863 Links on commons upload summaries do not link to commons
 OpenNoneT5498 Image history is confusing
 StalledNoneT96384 Integrate file revisions with description page history
 OpenFeatureNoneT39534 Wikimedia Commons should support searching by color
 DuplicateNoneT39535 Wikimedia Commons should support filtering by color
 ResolvedNoneT19503 Provide metadata support on Wikimedia Commons
 OpenNoneT122038 Moving/Deleting a video file does not move/delete subtitles files, nor does it inform the file mover of their existence
 OpenNoneT135221 Make TimedText content an integral part of the File page
 OpenFeatureNoneT2167 Use a dedicated interface for adding meta-data like interwiki links, rather than wikitext
 OpenNoneT132072 Integrate page meta-data as a new content model revision slot for consistency and atomicity
 ResolvedNoneT51662 VisualEditor: Use Multimedia/Wikidata's proposed rich structured meta-data in the image insertion dialog
 ResolvedNoneT68108 [Epic] Store media information for files on Wikimedia Commons as structured data
 OpenFeatureNoneT14963 allow per-page exceptions to spam blacklist
 OpenNoneT203157 Make the spam whitelist its own slot
 OpenFeatureNoneT56140 Move TemplateData to its own JSON-content namespace and associate with Template-namespace, or to its own TemplateData content model and revision slot
 StalledNoneT174043 Deploy Multi-Content Revisions
 OpenNoneT174022 Implement multi-content revisions
 InvalidNoneT198492 Create a maintenance script to drop rev_text_id and ar_text_id from the database.
 ResolvedNoneT231668 MCR schema migration
 ResolvedZabeT231674 [Epic] Remove support for writing to the pre-MCR schema
 ResolveddanielT198557 Remove the ability to write pre-MCR fields, limit the ability to read pre-MCR fields to migration scripts
 ResolvedCCicalese_WMFT242764 Test schema update from old versions to MCR-only schema
 ResolveddanielT231673 Make SCHEMA_COMPAT_NEW the default MCR migration schema
 ResolvedNoneT231672 Stop writing to pre-MCR fields in WMF production
 ResolvedNoneT231671 [EPIC] Ensure all direct or indirect access to pre-MCR fields is gated with the MCR migration stage and emits a warning if any pre-MCR schema fields are accessed
 ResolvedCCicalese_WMFT231693 No direct or indirect access to pre-MCR fields is ungated
 ResolveddanielT198341 Remove all references to the rev_text_id and ar_text_id fields
 Resolved• PcheloloT198342 Remove all usages of the 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo().
 ResolveddanielT198706 Make BackupDumper, WikiExporter and XmlDumpWriter compliant with the MCR revision retrival mechanism (main slot only)
 ResolvedBPirkleT203424 Replace the WikiExporter backup dump streaming mode with batched queries
 Resolved• PcheloloT228675 Remove direct access to the text table from the Translate extension.
 Resolved• PcheloloT228988 Create batch access interface for page content
 Resolved• PcheloloT233173 Make RevisionStore::newRevisionsFromBatch use BlobStore::getBlobBatch
 Resolved• PcheloloT230834 Create batch access interface for BlobStore
 ResolveddanielT234034 Create low level batch access interface for page content
 Resolved• PcheloloT233356 Flow: remove dependency on pre-MCR database schema
 Resolved• ClarakosiT233357 ReplaceText: remove dependency on pre-MCR database schema
 ResolveddanielT233358 SpamBlacklist: remove dependency on pre-MCR database schema
 ResolveddanielT238958 Remove pre-MCR schema fields during update
 ResolvedNoneT174047 Hide deprecated/unused fields on toolforge replica [MCR]

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
CCicalese_WMF moved this task from In Progress to Tech Debt on the Multi-Content-Revisions board.Oct 4 2018, 2:58 PM
CCicalese_WMF edited projects, added Multi-Content-Revisions (Tech Debt); removed Multi-Content-Revisions.
CCicalese_WMF edited projects, added Platform Team Workboards; removed Platform Team Legacy (Designing).Oct 14 2018, 4:07 PM
CCicalese_WMF moved this task from Not a real column to Ready on the Platform Team Workboards board.Dec 18 2018, 4:22 PM
daniel removed daniel as the assignee of this task.Jan 22 2019, 3:24 PM
daniel added a project: Release-Engineering-Team (Kanban).Feb 8 2019, 4:46 PM
greg subscribed.Feb 12 2019, 11:25 PM

@daniel: sorry, I don't see how/why this should go on the releng-kanban board?

daniel removed a project: Release-Engineering-Team (Kanban).Feb 13 2019, 11:39 AM

@daniel: sorry, I don't see how/why this should go on the releng-kanban board?

@greg it sholdn't - it should be on the CPT Kanban board. No idea what happened there, sorry :)

greg added a project: Multimedia.Mar 7 2019, 10:59 PM
MBinder_WMF removed a project: Multimedia.Mar 8 2019, 4:42 PM
BPirkle mentioned this in T198341: Remove all references to the rev_text_id and ar_text_id fields.Mar 13 2019, 10:04 PM
daniel updated the task description. (Show Details)Apr 2 2019, 4:13 PM
greg unsubscribed.Apr 2 2019, 6:53 PM
BPirkle subscribed.Apr 4 2019, 2:55 AM

Usages found:

  • includes/api/ApiQueryAllRevisions.php, line 85, in function run()
  • includes/api/ApiQueryRevisions.php, line 162, in function run() (if $this->fetchContent is true)
  • maintenance/testCompression.php line 51
  • tests/phpunit/includes/RevisionDbTestBase.php, line 1601, in function testGetRevisionText(), via data provider provideGetRevisionText()
  • tests/phpunit/includes/RevisionMcrReadNewDbTest.php, data provider provideGetRevisionText()
  • test/phpunit/includes/Revision/RevisionQueryInfoTest.php line 1054, in function testRevisionGetQueryInfo(), via data provider provideQueryInfo()
  • test/phpunit/includes/Revision/RevisionQueryInfoTest.php line 1068, in function testRevisionStoreGetQueryInfo(), via data provider provideQueryInfo()

The occurrences in the tests may be intentional?

daniel added a comment.Apr 4 2019, 9:14 AM
In T198342#5083815, @BPirkle wrote:

Usages found:

  • includes/api/ApiQueryAllRevisions.php, line 85, in function run()
  • includes/api/ApiQueryRevisions.php, line 162, in function run() (if $this->fetchContent is true)

As far as I can see, these can just be deleted. Your work on ApiQueryRevisionsBase has made them redundant.

  • maintenance/testCompression.php line 51

Can be deleted. The idea of setting the 'text' flag here is basically a pre-fetch: the old code in Revision would make use of old_text and friends directly if present in the row from a join, and would make a separate database query if not. The new code will always do a separate query if SCHEMA_COMPAT_READ_NEW is set, since there may be more than once slot to read. Any old_xxx fields passed to the constructor are ignored. Thus, the 'text' flag can just be deleted.

Note that the extra query isn't so terrible because we'd be doing a separate query to ExternalStore anyway. A bulk mode in BlobStore would still be nice to have.

By the way: while you are looking at testCompression.php, it would be nice to replace the usage of the deprecated Revision class with RevisionStore/RevisionRecord. This brings us back to having a way to access slot content without constructing a RevisionRecord. This wouldn't need much more than making RevisionStore::loadSlotRecords public. If you feel like it, go ahead :) Should probably have a separate ticket though...

  • tests/phpunit/includes/RevisionDbTestBase.php, line 1601, in function testGetRevisionText(), via data provider provideGetRevisionText()
  • tests/phpunit/includes/RevisionMcrReadNewDbTest.php, data provider provideGetRevisionText()

Of theRevisionDbTestBase subclasses, RevisionMcrReadNewDbTest and RevisionMcrWriteBothDbTest and RevisionPreMcrDbTest should test the 'text' flag, the others should not. In the base class, the provider should probably be abstract.

  • test/phpunit/includes/Revision/RevisionQueryInfoTest.php line 1054, in function testRevisionGetQueryInfo(), via data provider provideQueryInfo()
  • test/phpunit/includes/Revision/RevisionQueryInfoTest.php line 1068, in function testRevisionStoreGetQueryInfo(), via data provider provideQueryInfo()

Keep, since the relevant test case forces wgMultiContentRevisionSchemaMigrationStage to SCHEMA_COMPAT_OLD.

WDoranWMF assigned this task to BPirkle.Apr 15 2019, 3:01 PM
WDoranWMF set the point value for this task to 2.
WDoranWMF moved this task from Ready to Doing on the Platform Team Workboards board.
WDoranWMF edited projects, added Platform Team Workboards (Doing); removed Platform Team Workboards.
gerritbot added a comment.Apr 16 2019, 4:49 PM

Change 504369 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Remove usages of 'text' flag in revision-related getQueryInfo() calls

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

gerritbot added a comment.Apr 16 2019, 9:13 PM

Change 504369 merged by jenkins-bot:
[mediawiki/core@master] Remove usages of 'text' flag in revision-related getQueryInfo() calls

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

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.3; 2019-04-30).Apr 16 2019, 10:00 PM
daniel closed this task as Resolved.Apr 17 2019, 11:11 AM

Looks like this is done!

daniel reopened this task as Open.Apr 17 2019, 11:14 AM
daniel edited projects, added Platform Team Workboards (Waiting for Review); removed Platform Team Workboards (Doing).

Ooops, too fast, there is an open patch needing review: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/500755

daniel added a comment.Apr 17 2019, 12:04 PM

When this is done, please remember to enable the deprecation warning for 'text' flags in RevisionStore::getQueryInfo, see T200918. There is a TODO comment that references that task ID, so it's easy to find.

BPirkle added a comment.Apr 17 2019, 2:33 PM
In T198342#5118274, @daniel wrote:

Ooops, too fast, there is an open patch needing review: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/500755

That patch is for T198341, but this comment is on T198342.

However, I did just find three usages of getQueryInfo() with the 'text' flag in extensions:

  • SpamBlacklist/includes/BaseBlacklist.php
  • Translate/MessageCollection.php
  • Translate/translationaids/TranslationAidDataProvider.php

I'll see about fixing those.

daniel added a subscriber: Nikerabbit.Apr 17 2019, 3:10 PM
In T198342#5118842, @BPirkle wrote:
In T198342#5118274, @daniel wrote:

Ooops, too fast, there is an open patch needing review: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/500755

Hah, right.

That patch is for T198341, but this comment is on T198342.

However, I did just find three usages of getQueryInfo() with the 'text' flag in extensions:

  • SpamBlacklist/includes/BaseBlacklist.php
  • Translate/MessageCollection.php
  • Translate/translationaids/TranslationAidDataProvider.php

If you has questions about Translate, best talk to @Nikerabbit.

gerritbot added a comment.Apr 17 2019, 10:28 PM

Change 504797 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/extensions/SpamBlacklist@master] Remove usage of Revision::GetQueryInfo() with 'text' flag

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

CCicalese_WMF moved this task from Waiting for Review to Doing on the Platform Team Workboards board.Apr 29 2019, 3:01 PM
CCicalese_WMF edited projects, added Platform Team Workboards (Doing); removed Platform Team Workboards (Waiting for Review).
daniel closed subtask T198706: Make BackupDumper, WikiExporter and XmlDumpWriter compliant with the MCR revision retrival mechanism (main slot only) as Resolved.May 15 2019, 10:03 PM
CCicalese_WMF edited projects, added Platform Team Workboards (Team 3); removed Platform Team Workboards (Doing).Jul 1 2019, 2:52 PM
CCicalese_WMF moved this task from Backlog to Doing on the Platform Team Workboards (Team 3) board.
daniel updated the task description. (Show Details)Jul 17 2019, 10:57 AM
WDoranWMF edited projects, added Platform Team Workboards (Architecture Review Workboard); removed Platform Team Workboards (Team 3).Jul 17 2019, 4:20 PM
gerritbot added a comment.Jul 19 2019, 8:48 PM

Change 524593 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/Translate@master] Remove direct access to text table

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

Addshore unsubscribed.Jul 20 2019, 3:08 AM
daniel updated the task description. (Show Details)Jul 22 2019, 2:51 PM
daniel updated the task description. (Show Details)Jul 22 2019, 2:53 PM
daniel mentioned this in T198343: Replace all calls to Revision::getRevisionText().Jul 22 2019, 3:01 PM
daniel updated the task description. (Show Details)
daniel mentioned this in T228675: Remove direct access to the text table from the Translate extension..Jul 22 2019, 3:11 PM
daniel added a subtask: T228675: Remove direct access to the text table from the Translate extension..
daniel updated the task description. (Show Details)Jul 22 2019, 3:22 PM
Daimona subscribed.Jul 22 2019, 3:53 PM

So I see AF is mentioned here... I'm glad to help. To give you some context, we do access the text table directly in AbuseFilter::storeVarDump and AbuseFilter::loadVarDump, but that's not for getting revision text, as we save our custom data in the text table. It's also not related to Revision(Store)::getQueryInfo, so I don't know whether it should be handled in a separate task. Anyway, I hope to have saved you some time for investigation.

daniel added a comment.Jul 22 2019, 4:23 PM
In T198342#5354371, @Daimona wrote:

So I see AF is mentioned here... I'm glad to help. To give you some context, we do access the text table directly in AbuseFilter::storeVarDump and AbuseFilter::loadVarDump, but that's not for getting revision text

Oh, right! You could still use BlobStore instead of duplicating the code, but the way you are currently doing things is not a problem for the MCR schema migration.

daniel updated the task description. (Show Details)Jul 22 2019, 4:30 PM
• Fjalapeno moved this task from Backlog to Engineering Tasks Ready for Estimation on the Platform Team Workboards (Architecture Review Workboard) board.Jul 23 2019, 1:28 PM
• Fjalapeno moved this task from Engineering Tasks Ready for Estimation to Backlog on the Platform Team Workboards (Architecture Review Workboard) board.
• Fjalapeno removed BPirkle as the assignee of this task.Jul 23 2019, 6:23 PM
• Fjalapeno edited projects, added Platform Engineering; removed Platform Team Workboards (Architecture Review Workboard), Platform Engineering (MCR).
WDoranWMF raised the priority of this task from Medium to High.Jul 23 2019, 6:24 PM
WDoranWMF added a project: Platform Team Workboards (Clinic Duty Team).
WDoranWMF moved this task from Inbox to Later on the Platform Team Workboards (Clinic Duty Team) board.
WDoranWMF moved this task from Later to Ready (WIP:5) on the Platform Team Workboards (Clinic Duty Team) board.
WDoranWMF removed a project: Platform Engineering.
CCicalese_WMF added a project: Platform Engineering (MCR).Jul 23 2019, 9:20 PM
WDoranWMF lowered the priority of this task from High to Medium.Jul 24 2019, 9:47 PM
WDoranWMF moved this task from Ready (WIP:5) to Later on the Platform Team Workboards (Clinic Duty Team) board.
WDoranWMF moved this task from MCR to mop on the Platform Engineering board.Jul 26 2019, 6:38 PM
WDoranWMF edited projects, added Platform Team Initiatives (MCR); removed Platform Engineering (MCR).
CCicalese_WMF edited projects, added Platform Team Initiatives (MCR Schema Migration); removed Platform Team Initiatives (MCR).Aug 1 2019, 3:35 AM
WDoranWMF moved this task from Later to Inbox on the Platform Team Workboards (Clinic Duty Team) board.Aug 6 2019, 3:20 PM
Aklapper added a project: Structured-Data-Backlog.Aug 10 2019, 11:55 PM
• Ramsey-WMF moved this task from Triage to Tracking on the Structured-Data-Backlog board.Aug 13 2019, 7:27 PM
CCicalese_WMF removed a parent task: T200918: Make sure code that accesses legacy pre-MCR fields triggers warnings before switching off WRITE_OLD (compat) mode..Aug 30 2019, 11:51 PM
CCicalese_WMF edited projects, added Platform Team Workboards (Architecture Review Workboard); removed Platform Team Workboards (Clinic Duty Team).Sep 17 2019, 4:06 PM
WDoranWMF moved this task from Backlog to Next Sprint on the Platform Team Workboards (Architecture Review Workboard) board.Sep 17 2019, 4:07 PM
WDoranWMF moved this task from Next Sprint to Engineering Tasks Ready for Estimation on the Platform Team Workboards (Architecture Review Workboard) board.
WDoranWMF moved this task from Engineering Tasks Ready for Estimation to Ready on the Platform Team Workboards (Architecture Review Workboard) board.Sep 17 2019, 4:41 PM
daniel added a subtask: T233356: Flow: remove dependency on pre-MCR database schema.Sep 19 2019, 7:52 PM
daniel added a subtask: T233357: ReplaceText: remove dependency on pre-MCR database schema.
daniel moved this task from Ready to Blocked on the Platform Team Workboards (Architecture Review Workboard) board.Sep 19 2019, 8:07 PM
• Pchelolo closed subtask T233356: Flow: remove dependency on pre-MCR database schema as Resolved.Sep 19 2019, 10:29 PM
• Clarakosi closed subtask T233357: ReplaceText: remove dependency on pre-MCR database schema as Resolved.Sep 23 2019, 3:05 PM
• Pchelolo closed subtask T233358: SpamBlacklist: remove dependency on pre-MCR database schema as Resolved.Sep 23 2019, 6:47 PM
• Pchelolo closed subtask T228675: Remove direct access to the text table from the Translate extension. as Resolved.Oct 7 2019, 7:07 PM
• Pchelolo closed this task as Resolved.Oct 7 2019, 7:15 PM
• Pchelolo claimed this task.
• Pchelolo moved this task from Blocked to Done on the Platform Team Workboards (Architecture Review Workboard) board.
• Pchelolo subscribed.

With the completion of the Translate extension, all references has been either removed or gated. Resolving.

Hard deprecation is handled by T200918

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:41 PM
gerritbot added a comment.Jun 27 2021, 1:13 AM

Change 439976 abandoned by DannyS712:

[mediawiki/core@master] Replace calls to Revision::getQueryInfo with the 'text' option set.

Reason:

Revision::getQueryInfo method was removed entirely and linked task was resolved, hope its okay to abandon

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

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL