Paths

Table of Contentst

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

Authored by tiguchi on Oct 21 2022, 15:09.

Details

Reviewers

Cigaryno
Matthew

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15105: Cannot upload new mockups via Pholio

Commits

rPa8a38835e15e: Fix broken file PHID extraction that causes Pholio uploads to crash

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 created this revision.Oct 21 2022, 15:09

Owners added a reviewer: O1: Blessed Committers.Oct 21 2022, 15:09

Herald added subscribers: Cigaryno, valerio.bozzolan, tobiaswiese, speck. · View Herald TranscriptOct 21 2022, 15:09

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

Harbormaster completed remote builds in B92: Diff 285.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.

Cigaryno added a project: Pholio.Oct 21 2022, 15:55

Cigaryno mentioned this in Z1: Phorge.Nov 5 2022, 08:02

@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.

Matthew accepted this revision.Nov 11 2022, 20:09

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

Closed by commit rPa8a38835e15e: Fix broken file PHID extraction that causes Pholio uploads to crash. · Explain WhyNov 11 2022, 20:10

This revision was automatically updated to reflect the committed changes.

Matthew added a commit: rPa8a38835e15e: Fix broken file PHID extraction that causes Pholio uploads to crash.

valerio.bozzolan mentioned this in 2023 Week 2.Mar 13 2023, 18:26

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

valerio.bozzolan added inline comments.Mar 23 2023, 17:19

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

avivey mentioned this in 2023 Week 17.Apr 25 2023, 06:54

speck added inline comments.Jun 5 2023, 17:38

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.

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

pholio/

xaction/

PholioImageFileTransaction.php

8 lines

O1: Blessed Committers

Diff 285

View Options

src/applications/pholio/xaction/PholioImageFileTransaction.php

Fix broken file PHID extraction that causes Pholio uploads to crashClosedPublicActions