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

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
ResolvedBstorm
OpenNone
StalledNone
OpenNone
OpenBPirkle
OpenBPirkle
Resolveddaniel
ResolvedBPirkle
OpenNone
OpenNone
OpenNone
ResolvedAnomie
ResolvedAnomie
Resolvedtstarling
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
Resolveddaniel
ResolvedAnomie
ResolvedAnomie
ResolvedMarostegui
ResolvedAnomie
Resolvedtstarling
ResolvedAnomie
Resolveddaniel
ResolvedAnomie
ResolvedAnomie
ResolvedAnomie
Resolveddaniel
Resolveddaniel
Resolveddaniel
ResolvedNone

Event Timeline

daniel created this task.Jun 27 2018, 5:47 PM
daniel triaged this task as Normal priority.

Change 439976 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Replace calls to Revision::getQueryInfo with the 'text' option set.

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

Ladsgroup moved this task from incoming to monitoring on the Wikidata board.Jun 28 2018, 3:44 PM
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 set the point value for this task to 2.Apr 15 2019, 3:01 PM
WDoranWMF moved this task from Ready to Doing on the Core Platform Team Kanban board.
WDoranWMF assigned this task to BPirkle.

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