We didn't load enough data for this case for the custom view class Pholio uses. Fixes T6770. Re-jiggers the signature of the loadImages function in the PholioMockQuery to get there so as to not duplicate any business logic.
Details
Details
- Reviewers
epriestley chad - Maniphest Tasks
- T6770: Clicking "Show older changes" in Pholio produces PhabricatorDataNotAttachedException
- Commits
- Restricted Diffusion Commit
rPf24ae96bb6e0: Pholio - fix show older transactions for if there are inline comments
made a pholio mock with lots of inline comments. pre-patch "show older" fatals and post-patch "show older" works
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I wonder if we should eventually formalize the idea of "attachment points" and provide some more standard way to fill in missing attachments. The attachX() + getX() + needX() approach generally seems to work pretty well, but it definitely falls down a bit in cases like this.
We had some ad-hoc loadAndAttach() type things before, but they were all custom and nonstandard and operated on only a single object. I also don't like the`loadRelatives()` approach much -- it always felt too magical and lacked power once we implemented policies.
If "attachable" properties were slightly more formal, Query classes could potentially define how to load each type of attachment and then a common API could say "load attachment X for these objects" and be used on both the primary Query pathway and cases like this where we need to fill data in later.
Anyway, not a big deal (this doesn't come up often) and I don't have any concrete ideas on an implementation yet, but it's something I imagine maybe coming up with an approach for in the next year or so.