Page MenuHomePhabricator

Redesign DBAccessObjectUtils
Closed, ResolvedPublic

Description

This is one of the most counter-intuitive APIs of the rdbms library. Some issues that comes to mind:

  • ::getDBOptions() does both what to query (replica or primary) and how to query (query options). These are separate logical pieces.
  • The method returns $options that would be fed into query builders or merged with existing options array. This is what we are trying to deprecate.
  • Also the method returns DB_* constants which is also what we are trying to deprecate (passing around the constant)
  • the same method provides fallback options when READ_LATEST_IMMUTABLE is provided, this is only used in one place and no more. Actually in place it actively removes this flag if it's provided by higher callers. Fallback options have been thrown away in all cases (but the sql blob store which I'm not even sure it really needs it).
  • This is the weirdest to me: There is an interface that has these constants (IDBAccessObject), most classes using DBAccessObjectUtils implement the interface and call self:: on it. That is extremely confusing to a newcomer, not done in similar way in any other interface I know.

Proposed solution:

  • Keep IDBAccessObject but just start calling the constants instead of classes implementing the interface.
  • Get rid of the fallback logic, just copy-paste the logic to SqlBlobStore. And get rid of ::READ_LATEST_IMMUTABLE
  • Create a new method ::getDBFromFlag combining getDBOptions and getDBFromIndex, taking a flag var and ICP and returning an IRDB.
  • Add a method to SQB like ->lockingFlags() that would take the bitfield flag and set the locking options.
  • Deprecate and remove ::getDBOptions and ::getDBFromIndex
  • Document the decision and possibly use linting rules to enforce them.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/coremaster+4 -0
mediawiki/coremaster+58 -48
mediawiki/coremaster+22 -32
mediawiki/extensions/AbuseFiltermaster+96 -137
mediawiki/extensions/CentralAuthmaster+42 -25
mediawiki/extensions/GrowthExperimentsmaster+36 -28
mediawiki/extensions/WikimediaEditorTasksmaster+12 -13
mediawiki/extensions/MachineVisionmaster+14 -11
mediawiki/extensions/PageAssessmentsmaster+30 -30
mediawiki/extensions/ReadingListsmaster+25 -14
mediawiki/extensions/Thanksmaster+8 -12
mediawiki/coremaster+52 -31
mediawiki/extensions/UserMergemaster+2 -2
mediawiki/coremaster+52 -48
mediawiki/extensions/GrowthExperimentsmaster+4 -10
mediawiki/coremaster+5 -2
mediawiki/coremaster+75 -74
mediawiki/extensions/Translatemaster+19 -13
mediawiki/extensions/FileImportermaster+3 -2
mediawiki/extensions/PageTriagemaster+6 -3
mediawiki/extensions/CirrusSearchmaster+4 -3
mediawiki/extensions/TwoColConflictmaster+2 -2
mediawiki/extensions/ConfirmEditmaster+2 -2
mediawiki/extensions/CampaignEventsmaster+19 -13
mediawiki/extensions/AbuseFiltermaster+19 -14
mediawiki/extensions/EventBusmaster+5 -4
mediawiki/extensions/GlobalPreferencesmaster+2 -1
mediawiki/extensions/Echomaster+3 -3
mediawiki/extensions/BounceHandlermaster+1 -1
mediawiki/extensions/NewUserMessagemaster+2 -1
mediawiki/extensions/PageAssessmentsmaster+3 -3
mediawiki/extensions/ProofreadPagemaster+4 -2
mediawiki/extensions/Wikibasemaster+5 -4
mediawiki/extensions/OAuthmaster+11 -9
mediawiki/extensions/Flowmaster+4 -4
mediawiki/extensions/IPInfomaster+4 -2
mediawiki/extensions/GlobalUsagemaster+5 -3
mediawiki/extensions/Wikistoriesmaster+2 -1
mediawiki/extensions/MediaModerationmaster+7 -7
mediawiki/extensions/WikiLambdamaster+3 -2
mediawiki/extensions/CentralAuthmaster+2 -2
mediawiki/extensions/ReadingListsmaster+11 -11
mediawiki/extensions/WikimediaMaintenancemaster+2 -2
mediawiki/extensions/LdapAuthenticationmaster+1 -1
mediawiki/extensions/FlaggedRevsmaster+32 -34
mediawiki/extensions/CentralAuthmaster+59 -54
mediawiki/extensions/GrowthExperimentsmaster+77 -59
mediawiki/coremaster+233 -211
mediawiki/coremaster+66 -64
mediawiki/coremaster+39 -24
mediawiki/coremaster+75 -75
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 993131 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/WikimediaMaintenance@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993132 had a related patch set uploaded (by Gerrit maintenance bot; author: Gerrit maintenance bot):

[mediawiki/extensions/Wikistories@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993119 merged by jenkins-bot:

[mediawiki/extensions/LdapAuthentication@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993131 merged by jenkins-bot:

[mediawiki/extensions/WikimediaMaintenance@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993126 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993109 abandoned by Ladsgroup:

[mediawiki/extensions/CentralAuth@master] Remove indirect calls to IDBAccessObject::READ_* constants

Reason:

False positive

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

Change 993129 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993132 merged by jenkins-bot:

[mediawiki/extensions/Wikistories@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993120 merged by jenkins-bot:

[mediawiki/extensions/MediaModeration@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993118 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993117 merged by jenkins-bot:

[mediawiki/extensions/GlobalUsage@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993122 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993115 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993130 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993112 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993125 merged by jenkins-bot:

[mediawiki/extensions/ProofreadPage@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993123 merged by jenkins-bot:

[mediawiki/extensions/PageAssessments@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993107 merged by jenkins-bot:

[mediawiki/extensions/BounceHandler@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993121 merged by jenkins-bot:

[mediawiki/extensions/NewUserMessage@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993116 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993113 merged by jenkins-bot:

[mediawiki/extensions/EventBus@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993106 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993108 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993111 merged by jenkins-bot:

[mediawiki/extensions/ConfirmEdit@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993110 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993128 merged by jenkins-bot:

[mediawiki/extensions/TwoColConflict@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993114 merged by jenkins-bot:

[mediawiki/extensions/FileImporter@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993124 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 993127 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change 992947 merged by jenkins-bot:

[mediawiki/core@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

I guess it's a bit late to comment on it now but IMO it would have made more sense to wait for PHP 8 and replace it with an enum like Recency::Read rather than to make people use an unhelpful name like IDBAccessObject everywhere in their code. That would have also allowed for a proper deprecation process since you can differentiate between an enum and the string representation of the enum.

They are not mutually exclusive, we can eventually do that. For now, I really need to get rid of the IDBAccessObject implementation.

Change 1003388 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Hard-deprecate DBAccessObjectUtils::getDBFromIndex

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

Change 1003392 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/GrowthExperiments@master] Switch away from DBAccessObjectUtils::getDBOptions()

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

Change 1003388 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Hard-deprecate DBAccessObjectUtils::getDBFromIndex

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

Change 1003768 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Remove more usages of DBAccessObjectUtils::getDBOptions()

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

Change 1004615 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Remove IDBAccessObject from being implemented in many classes

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

Change 1003392 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Switch away from DBAccessObjectUtils::getDBOptions()

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

Change 1004615 merged by jenkins-bot:

[mediawiki/core@master] Remove IDBAccessObject from being implemented in many classes

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

Change 1005577 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/UserMerge@master] Follow up on core IDBAccessObject changes

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

Change 1005577 merged by jenkins-bot:

[mediawiki/extensions/UserMerge@master] Follow up on core IDBAccessObject changes

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

Change 1003768 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Remove more usages of DBAccessObjectUtils::getDBOptions()

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

Change 1007325 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/Thanks@master] Switch to ICP and stop using DBAccessObjectUtils::getDBOptions()

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

Change 1007572 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/ReadingLists@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1007572 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1007325 merged by jenkins-bot:

[mediawiki/extensions/Thanks@master] Switch to ICP and stop using DBAccessObjectUtils::getDBOptions()

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

Change 1007975 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/PageAssessments@master] Switch to ICP and stop using DBAccessObjectUtils::getDBOptions()

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

Change 1007975 merged by jenkins-bot:

[mediawiki/extensions/PageAssessments@master] Switch to ICP and stop using DBAccessObjectUtils::getDBOptions()

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

Change 1008566 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/MachineVision@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1008566 merged by jenkins-bot:

[mediawiki/extensions/MachineVision@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1008846 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/WikimediaEditorTasks@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1008851 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/GrowthExperiments@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1008846 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEditorTasks@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1008851 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1012388 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/AbuseFilter@master] FilterLookup: Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1013028 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/CentralAuth@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change 1013028 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Stop using DBAccessObjectUtils::getDBOptions()

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

Change #1013578 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Move away from DBAccessObjectUtils::getDBOptions in more places

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

Change #1012388 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] FilterLookup: Stop using DBAccessObjectUtils::getDBOptions()

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

Change #1013578 merged by jenkins-bot:

[mediawiki/core@master] Move away from DBAccessObjectUtils::getDBOptions in more places

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

Change #1014630 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] Remove last uses of DBAccessObjectUtils::getDBOptions()

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

Change #1014630 merged by jenkins-bot:

[mediawiki/core@master] Remove last uses of DBAccessObjectUtils::getDBOptions()

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

Change #1026262 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/core@master] rdbms: Hard-deprecate DBAccessObjectUtils::getDBOptions()

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

Change #1026262 merged by jenkins-bot:

[mediawiki/core@master] rdbms: Hard-deprecate DBAccessObjectUtils::getDBOptions()

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

Ladsgroup moved this task from In progress to Done on the DBA board.

Change #1063334 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/BlueSpiceFlaggedRevsConnector@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change #1063335 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/BlueSpiceFoundation@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change #1063336 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/CommentStreams@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Change #1063337 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/ContentStabilization@master] Remove indirect calls to IDBAccessObject::READ_* constants

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

Maybe better to create a separate ticket and connect these patches to that. These patches are not really related to "Redesign DBAccessObjectUtils" and the ticket is closed as resolved.