Paths

Table of Contentst

Differential D19814

Continue cleaning up queries in the wake of changes to "%Q"
ClosedPublic
Actions

Authored by epriestley on Nov 15 2018, 2:00 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13216: 2018 Week 45-47 Bonus Content
T13217: Upgrading: Hardening of qsprintf()

Commits

rP933462b4873b: Continue cleaning up queries in the wake of changes to "%Q"

Summary

Depends on D19810. Ref T13217. Ref T13216. I mostly used grep implode | grep OR and grep implode | grep AND to find these -- not totally exhaustive but should be a big chunk of the callsites that are missing %LO / %LA.

Test Plan

These are tricky to test exhaustively, but I made an attempt to hit most of them:

Browsed Almanac interfaces.
Created/browsed Calendar events.
Enabled/disabled/showed the lock log.
Browsed repositories.
Loaded Facts UI.
Poked at Multimeter.
Used typeahead for users and projects.
Browsed Phriction.
Ran various fulltext searches.

Not sure these are reachable:

All the lint stuff might be dead/unreachable/nonfunctional?

Diff Detail

Repository

rP Phabricator

Branch

query7

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 21165
Build 28780: Run Core Tests
Build 28779: arc lint + arc unit

Event Timeline

epriestley created this revision.Nov 15 2018, 2:00 PM

Herald added a subscriber: yelirekim. · View Herald TranscriptNov 15 2018, 2:00 PM

Harbormaster completed remote builds in B21165: Diff 47316.Nov 15 2018, 2:02 PM

epriestley requested review of this revision.Nov 15 2018, 2:02 PM

epriestley added a child revision: D19816: Unify intracluster sync and Drydock working copy construction timeouts as a repository "copy time limit".Nov 16 2018, 5:19 PM

amckinley accepted this revision.Nov 16 2018, 7:36 PM

amckinley added inline comments.

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
1749	We clearly need an`%L+` operator for this extremely common pattern.

This revision is now accepted and ready to land.Nov 16 2018, 7:36 PM

Haha, yeah. Differential had one UNION ALL too but %LU seemed a little ridiculous.

MySQL has a SUM() function but it only works on aggregates and I couldn't find any way to express this as ADD_THESE_UP(%LQ).

Closed by commit rP933462b4873b: Continue cleaning up queries in the wake of changes to "%Q" (authored by epriestley). · Explain WhyNov 16 2018, 8:49 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

applications/

almanac/

query/

AlmanacInterfaceQuery.php

2 lines

calendar/

query/

PhabricatorCalendarEventQuery.php

4 lines

daemon/

management/

PhabricatorLockLogManagementWorkflow.php

4 lines

diffusion/

DiffusionLintSaveRunner.php

8 lines

conduit/

DiffusionBrowseQueryConduitAPIMethod.php

4 lines

controller/

DiffusionLintController.php

12 lines

query/

DiffusionCachedResolveRefsQuery.php

4 lines

fact/

query/

PhabricatorFactDatapointQuery.php

4 lines

multimeter/

controller/

MultimeterSampleController.php

8 lines

packages/

query/

PhabricatorPackagesQuery.php

2 lines

people/

query/

PhabricatorPeopleQuery.php

2 lines

phrequent/

query/

PhrequentUserTimeQuery.php

4 lines

phriction/

query/

PhrictionDocumentQuery.php

8 lines

project/

query/

PhabricatorProjectQuery.php

2 lines

infrastructure/

query/

policy/

PhabricatorCursorPagedPolicyAwareQuery.php

35 lines

Diff 47316

View Options

src/applications/almanac/query/AlmanacInterfaceQuery.php

View Options

src/applications/calendar/query/PhabricatorCalendarEventQuery.php

View Options

src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php

View Options

src/applications/diffusion/DiffusionLintSaveRunner.php

View Options

src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php

View Options

src/applications/diffusion/controller/DiffusionLintController.php

View Options

src/applications/diffusion/query/DiffusionCachedResolveRefsQuery.php

View Options

src/applications/fact/query/PhabricatorFactDatapointQuery.php

View Options

src/applications/multimeter/controller/MultimeterSampleController.php

View Options

src/applications/packages/query/PhabricatorPackagesQuery.php

View Options

src/applications/people/query/PhabricatorPeopleQuery.php

View Options

src/applications/phrequent/query/PhrequentUserTimeQuery.php

View Options

src/applications/phriction/query/PhrictionDocumentQuery.php

View Options

src/applications/project/query/PhabricatorProjectQuery.php

View Options

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

Continue cleaning up queries in the wake of changes to "%Q"ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 47316

src/applications/almanac/query/AlmanacInterfaceQuery.php

src/applications/calendar/query/PhabricatorCalendarEventQuery.php

src/applications/daemon/management/PhabricatorLockLogManagementWorkflow.php

src/applications/diffusion/DiffusionLintSaveRunner.php

src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php

src/applications/diffusion/controller/DiffusionLintController.php

src/applications/diffusion/query/DiffusionCachedResolveRefsQuery.php

src/applications/fact/query/PhabricatorFactDatapointQuery.php

src/applications/multimeter/controller/MultimeterSampleController.php

src/applications/packages/query/PhabricatorPackagesQuery.php

src/applications/people/query/PhabricatorPeopleQuery.php

src/applications/phrequent/query/PhrequentUserTimeQuery.php

src/applications/phriction/query/PhrictionDocumentQuery.php

src/applications/project/query/PhabricatorProjectQuery.php

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

Continue cleaning up queries in the wake of changes to "%Q"
ClosedPublic
Actions

Revision Contents
Changeset List