Check that min epoch < max epoch in PhabricatorFeedQuery::withEpochInRange()
ClosedPublic
Actions

Authored by aklapper on Wed, Feb 19, 10:15.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Commits

rPabeeadd6b013: Check that min epoch < max epoch in PhabricatorFeedQuery::withEpochInRange()

Summary

PhabricatorFeedQuery::withEpochInRange() returns zero results when passing parameters in the wrong order.
Instead return a PhutilArgumentUsageException which makes it clear why there are no results.

Test Plan

On an early morning without coffee supply, write custom code like

$query = id(new PhabricatorFeedQuery())
         ->setViewer(PhabricatorUser::getOmnipotentUser())
         ->withFilterPHIDs(array($user->getPHID()))
         ->withEpochInRange(time(), time() - 86400)
         ->setReturnPartialResultsOnOverheat(true);
$stories = $query->execute();

and wonder why you get zero results. Optionally, feel stupid for a moment.
Apply patch, now get an "Unhandled Exception ("PhutilArgumentUsageException") - Minimum range must be lower than maximum range."

Diff Detail

Repository

rP Phorge

Branch

feedEpochRange (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1736
Build 1736: arc lint + arc unit

Event Timeline

aklapper created this revision.Wed, Feb 19, 10:15

Owners added a reviewer: O1: Blessed Committers.Wed, Feb 19, 10:15

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptWed, Feb 19, 10:15

Harbormaster completed remote builds in B1732: Diff 2680.Wed, Feb 19, 10:15

aklapper requested review of this revision.Wed, Feb 19, 10:15

sgtm

This revision is now accepted and ready to land.Wed, Feb 19, 12:57

Small question. Is it supposed to be used with null+null?

valerio.bozzolan added inline comments.Wed, Feb 19, 13:20

src/applications/feed/query/PhabricatorFeedQuery.php
26	Maybe this, to support null on one, or on the other, from unknown usages / unknown resetters

Uh, nice thought! Phorge accepts and handles correctly one or two parameters being null which I think is fine as-is, so I'd only check for switched parameters created by confused developers here.

Oh wait I totally misunderstood you, sigh

Alright, even after calling ->withEpochInRange(null, null) this very code change (comparing null with null) works fine without a problem, so I'd not add that additional null check

I don't know if anybody is supposed to run