Page MenuHomePhabricator

Remove all usages of the 'text' flag in calls to Revision::getQueryInfo() and RevisionStore::getQueryInfo().
Open, NormalPublic2 Story Points

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 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

Related Objects

StatusAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
OpenNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Openholger.knust
OpenNone
Resolveddaniel
ResolvedBPirkle
OpenPchelolo
ResolvedPchelolo
OpenClarakosi
Opendaniel

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
daniel removed daniel as the assignee of this task.Jan 22 2019, 3:24 PM
greg added a subscriber: greg.Feb 12 2019, 11:25 PM

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

@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
daniel updated the task description. (Show Details)Apr 2 2019, 4:13 PM
greg removed a subscriber: greg.Apr 2 2019, 6:53 PM
BPirkle added a subscriber: BPirkle.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

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 Core Platform Team Workboards board.

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

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

daniel closed this task as Resolved.Apr 17 2019, 11:11 AM

Looks like this is done!

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.

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.

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.

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

daniel updated the task description. (Show Details)Jul 17 2019, 10:57 AM

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

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 updated the task description. (Show Details)Jul 22 2019, 3:22 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.

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
WDoranWMF raised the priority of this task from Normal to High.Jul 23 2019, 6:24 PM
WDoranWMF removed a project: Core Platform Team.
WDoranWMF lowered the priority of this task from High to Normal.Jul 24 2019, 9:47 PM
WDoranWMF moved this task from MCR to mop on the Core Platform Team board.Jul 26 2019, 6:38 PM