...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.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T4712: Paginate transactions on objects once there are a huge number of them
- Commits
- Restricted Diffusion Commit
rP3fd16a9ba553: Transactions - introduce a buildTransactionTimeline function
viewed maniphest tasks and they looked as spiffy as ever.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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. | |
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
updates as discussed. api now takes three parameters, first two required and the third ($engine) defaults to null