Page MenuHomePhabricator

Transactions - adding willRenderTimeline to handle tricky cases
ClosedPublic

Authored by btrahan on Dec 4 2014, 9:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 10:10 PM
Unknown Object (File)
Mon, Mar 25, 10:10 PM
Unknown Object (File)
Mon, Mar 25, 7:59 PM
Unknown Object (File)
Mon, Mar 25, 7:13 PM
Unknown Object (File)
Mon, Mar 25, 6:21 PM
Unknown Object (File)
Mon, Mar 25, 4:55 PM
Unknown Object (File)
Sun, Mar 24, 1:08 AM
Unknown Object (File)
Sat, Mar 23, 3:10 PM
Subscribers

Details

Summary

Fixes T6693.

Test Plan

Made a bunch of comments on a diff with differential, being sure to leave inlines here and there. This reproduced the issue in T6693. With this patch this issue no longer reproduces!

Successfully "showed older changes" in Maniphest too.

Diff Detail

Repository
rP Phabricator
Branch
T6693
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 3181
Build 3187: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

btrahan retitled this revision from to Transactions - adding willRenderTimeline to handle tricky cases.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

deploy it all over the place

epriestley edited edge metadata.
This revision is now accepted and ready to land.Dec 4 2014, 9:57 PM
This revision was automatically updated to reflect the committed changes.

Oh, another thought on this "fix it in 83 places" issue is that Traits (available from PHP 5.4.0 forward) would be a better fit for most of the things we use interfaces for -- they're basically interfaces which let you provide a default implementation. If this was a trait instead, you'd define a default implementation and then just specialize it where necessary.

We'd lose support for PHP 5.2.x and 5.3.x and only get slightly cleaner code, so I'm not in a rush to jump on this, but I imagine we'll eventually bump the minimum supported PHP version. Currently, I think we'd basically get:

  • Traits replace most interfaces, so we'd have a bit less code duplication.
  • Closures could simplify the exception handling in unit tests a little bit.

Not terribly compelling.

We could also adopt a reflection-based calling convention, like:

maybe_call($object, 'willRenderTimeline', $arg, $arg);

...which would do:

if (method_exists($object, $call)) {
  return call_user_func_array(...);
}

But I think that makes a more painful long-term mess than copy/pasting 83 times, which is tedious but straightforward, statically analyzable, etc., and which we can eventually get away from with traits.

Generally speaking, I don't mind doing this sort of thing, though I am likely to tackle other tasks in my queue first... Otherwise, under the context of fixing a live bug I do wish we had traits... :D