Page MenuHomePhabricator

Transactions - adding willRenderTimeline to handle tricky cases

Authored by btrahan on Dec 4 2014, 9:18 PM.
Referenced Files
Unknown Object (File)
Wed, Feb 7, 5:50 AM
Unknown Object (File)
Jan 31 2024, 3:08 PM
Unknown Object (File)
Jan 28 2024, 10:40 PM
Unknown Object (File)
Jan 28 2024, 4:51 AM
Unknown Object (File)
Jan 24 2024, 7:39 AM
Unknown Object (File)
Jan 10 2024, 5:04 PM
Unknown Object (File)
Jan 1 2024, 7:06 PM
Unknown Object (File)
Jan 1 2024, 7:06 PM



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

rP Phabricator
Lint Not Applicable
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