Page MenuHomePhabricator

Transactions - adding willRenderTimeline to handle tricky cases
ClosedPublic

Authored by btrahan on Dec 4 2014, 9:18 PM.
Tags
None
Attached Files
Unknown Object (File)
Apr 14 2017, 12:53 AM
Unknown Object (File)
Apr 12 2017, 5:54 PM
Unknown Object (File)
Apr 10 2017, 4:29 PM
Unknown Object (File)
Feb 8 2017, 10:32 PM
Unknown Object (File)
Jan 20 2017, 9:20 PM
Unknown Object (File)
Nov 19 2016, 2:15 PM
Unknown Object (File)
Nov 16 2016, 9:38 PM
Unknown Object (File)
Nov 12 2016, 1:15 AM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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