Page MenuHomePhabricator

Transactions - introduce a buildTransactionTimeline function
ClosedPublic

Authored by btrahan on Nov 12 2014, 10:33 PM.
Tags
None
Attached Files
Unknown Object (File)
Jan 20 2017, 8:25 PM
Unknown Object (File)
Dec 30 2016, 1:19 AM
Unknown Object (File)
Nov 4 2016, 3:36 PM
Unknown Object (File)
Nov 2 2016, 4:55 AM
Unknown Object (File)
Oct 23 2016, 12:38 AM
Unknown Object (File)
Oct 19 2016, 6:09 AM
Unknown Object (File)
Sep 14 2016, 3:46 PM
Unknown Object (File)
Sep 13 2016, 3:09 AM
Subscribers

Details

Summary

...way way down in PhabricatorController. Use it on ManiphestTaskDetailController to test it. Ref T4712. I think the pager logic to be added as part of T4712 can safely reside entirely within this method. As I said earlier, 5 parameters is a lot, so I don't really want to add more. Next diff would do the pagination logic and the diff after that would deploy it everywhere. If while deploying it everywhere I find something off, that will be a different diff.

Test Plan

viewed maniphest tasks and they looked as spiffy as ever.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Transactions - introduce a buildTransactionTimeline function.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.

Some notes on reducing parameter count.

src/applications/base/controller/PhabricatorController.php
530

I think we could safely use getViewer() to pull this.

531

We could make this a PhabricatorApplicationTransactionInterface, I think. This simplifies a few other things a little...

533

You can potentially figure out the right $view by calling:

$xaction = $object->getApplicationTransactionTemplate();
$view = $xaction->getApplicationTransactionViewObject();

...to simplify this a bit.

534

I think it's safe to omit this. We build one by default if it isn't passed in, and I believe no callsites do anything special with it.

542–549

We'll do this automatically if you don't pass an engine, so you can just omit it, I think.

551

This should use $view (or we should derive $view earlier and then use it here).

555

Can just omit engine, I believe.

This revision is now accepted and ready to land.Nov 13 2014, 6:30 PM

Thanks - that all makes sense to me except the engine bit I am not sure still.

I wanted to keep the engine in the controller and pass it down because I believe there's a big query somewhere in that process function. The controllers currently do things like use the engine for customfields or in the maniphest case something like description. I guess then the "worst case" is just changing 1 query into 2 which isn't so bad.

So is there some query inside and is it okay to transform it from 1 to 2 queries as part of this change?

We do lose some potential efficiency by omitting $engine. For example, on a revision, we'll run one engine to markup the summary/test plan, and then a separate engine for the comments. These could be batched and performance would improve slightly.

However, the gain would probably be very small in most cases, I don't think we ever actually do this in controllers today, and we'd have to separate things quite a bit to get all the fields added to the engine first before it got passed down anywhere.

In this case, where the engine isn't being used for anything else (I think? I don't see any other uses...), there's no difference between doing it manually vs omitting it (we just do the exact same thing internally if you omit it). If the engine was also being used to render custom fields this would be a regression, but I think we never do that.

I think at least making it optional (e.g. PhabricatorMarkupEngine $engine = null) probably lets us remove a bunch of code with zero performance impact today, and leaves the door open for possibly picking up those optimizations in the future. So maybe that's an approach? But we could also just retool this API if we ever want to pursue that performance.

Oh, we do actually batch DESCRIPTION into this. But we shouldn't really -- it should more properly be handled by CustomField, which doesn't share the engine.

Doing an optional $engine = null to retain the batching here for now is maybe reasonable, then? Pretty sure none of the other engines batch anything.

Sorry, bad reading comprehension on my part.

btrahan edited edge metadata.

updates as discussed. api now takes three parameters, first two required and the third ($engine) defaults to null

...also handle the null engine case...

...really handle null engine case.

This revision was automatically updated to reflect the committed changes.