Page MenuHomePhorge

Fix broken file PHID extraction that causes Pholio uploads to crash
ClosedPublic

Authored by tiguchi on Oct 21 2022, 15:09.
Tags
Referenced Files
F2055911: D25058.id.diff
Thu, Mar 28, 00:51
F2054648: D25058.diff
Wed, Mar 27, 18:44
Unknown Object (File)
Sun, Mar 24, 05:40
Unknown Object (File)
Wed, Mar 20, 21:17
Unknown Object (File)
Wed, Mar 20, 09:53
Unknown Object (File)
Feb 25 2024, 07:33
Unknown Object (File)
Feb 25 2024, 07:33
Unknown Object (File)
Feb 25 2024, 06:52

Details

Summary

A commit earlier this year modified the structure of the file upload transaction data value, by nesting the array of file upload PHIDs in another array.
The extractFilePHIDs method was not updated to deal with that change though, therefore new mock uploads via Pholio would crash.
This patch fixes that method so it can process the updated transaction data.

Resolves T15105

Test Plan

Patched my live Phabricator installation with this fix and successfully uploaded new Pholio mockups.

Diff Detail

Repository
rP Phorge
Branch
fix/pholio-upload-crash/T15105
Lint
Lint Passed
Unit
Tests Skipped
Build Status
Buildable 92
Build 92: arc lint + arc unit

Event Timeline

tiguchi requested review of this revision.Oct 21 2022, 15:09

The harbormaster has passed and you have properly fixed the bug.

NOTE: This revision is not ready to land because I am not a member of Blessed Committers. It will be ready to land once accepted by members of Blessed Committers.

@tiguchi you don't have to add reviewers yourself.

Well, I did ask for this to be converted into a revision, so I'm OK getting added as a reviewer. Sorry for the delay, I've been on vacation.

This revision is now accepted and ready to land.Nov 11 2022, 20:09

After your change, it seems Evan fixed this as well, but with a different approach:

https://secure.phabricator.com/rPa83cb99e856a70ac355dc51547949b50485bb768

It seems Evan created an array indexed by PHID, so to don't process a PHID twice I think

src/applications/pholio/xaction/PholioImageFileTransaction.php
110–118

Note that Evan fixed with this version ↑

https://secure.phabricator.com/rPa83cb99e856a70ac355dc51547949b50485bb768

I post this comment so we can compare

src/applications/pholio/xaction/PholioImageFileTransaction.php
110–118

We should probably update to match. I agree it's likely the upstream fix was done to avoid loading a pholio image multiple times.