Paths

Table of Contentst

Differential D25441

Fix possible array to string conversion renaming Pholio Mockup image
ClosedPublic
Actions

Authored by aklapper on Sep 26 2023, 13:10.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15646: Renaming several Pholio Mockup images creates neverending daemon task (PhutilProxyException due to Array to string conversion)

Commits

rPf42dd5819e98: Fix possible array to string conversion renaming Pholio Mockup image

Summary

Premising that the $old and $new variables are 1-element arrays defined as PHID=>title,
this can cause renderValue() repeatedly fail when passing an array instead of its value.

Thus pass head($old) instead, to get the first value - that is the only one, even if you rename
multiple images (since this Transaction is about a single Mockup image).

Closes T15646

Test Plan

Have phd running
Create a Pholio mockup with at least one image

Edit the Pholio mockup and:

rename the Titles of an image
rename a single Image
rename no image

No nuclear implosions. You still see a lovely Feed mentioning each rename.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Sep 26 2023, 13:10

Owners added a reviewer: O1: Blessed Committers.Sep 26 2023, 13:10

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptSep 26 2023, 13:10

Harbormaster completed remote builds in B850: Diff 1396.Sep 26 2023, 13:10

aklapper requested review of this revision.Sep 26 2023, 13:10

Thanks!

Maybe we can expand the test plan:

rename a single file in the Pholio
create a new Pholio with one file

Where to find the test plan? (And is expanding a hard requirement to get this reviewed?)

valerio.bozzolan edited the test plan for this revision. (Show Details)Nov 10 2023, 12:27

In D25441#12816, @aklapper wrote:

Where to find the test plan?

We can improve the Test Plan from Edit Revision → Test Plan to help other people to reproduce the crash

Unfortunately I cannot reproduce the crash or the error log so I'm just trying to understand what is happening here.

Before this change, if I create a Pholio Mock and if I rename the image titles, this happens (the message is a bit weird since the default title is empty but it's unrelated I think):

valerio.bozzolan renamed an image (3) from to ASD.
valerio.bozzolan renamed an image (2) from to LOL.
valerio.bozzolan renamed an image (3) from to LEL.
valerio.bozzolan renamed an image (4) from to LÆL.
valerio.bozzolan renamed an image (5) from to EHE.
valerio.bozzolan renamed an image (3) from to AHA.

Same, if I rename a single image again:

valerio.bozzolan renamed an image (EHE) from AHA to EHE.

Maybe I have to test this against PHP 8.1

Maybe I have to test this against PHP 8.1

Hmm, this happened in our production instance still running PHP 7.2 (I think)

Since this is for the timeline text maybe it should check for array and just say “multiple images” rather than grabbing the first.

I'm now quite sure that $old and $new always are array, and each one consists in a single element with key PHID and value => title.

So I'm 99.99% sure that $old and $new always are PHID=>title array and always with count($that) === 1 - whenever you rename a single photo or multiple ones. So, I see this as totally OK.

I just don't think that this happens renaming multiple photos. I think this just happens renaming a single photo, in a very unclear case that triggers an email about this. So I'm inclined to rename this.

src/applications/pholio/xaction/PholioImageNameTransaction.php
21	↑ Interesting line
34–36	The cast to string is very probably not necessary for rendering this. In the case the value is missing or it's null, it's safe to call `renderValue(null)` as far as I can test.

This revision is now accepted and ready to land.Nov 13 2023, 17:23

valerio.bozzolan retitled this revision from Fix array to string conversion renaming several Pholio mockup images at once to Fix possible array to string conversion renaming Pholio Mockup image.Nov 13 2023, 17:27

valerio.bozzolan edited the summary of this revision. (Show Details)

valerio.bozzolan edited the test plan for this revision. (Show Details)

Thanks for the deeper investigation! I can confirm that with the last version and following my original steps to reproduce, no stuck daemon task about the mock looping due to an exception on http://phorge.localhost/daemon/ anymore, so I'm going to land this.

Closed by commit rPf42dd5819e98: Fix possible array to string conversion renaming Pholio Mockup image. · Explain WhyNov 16 2023, 09:42

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPf42dd5819e98: Fix possible array to string conversion renaming Pholio Mockup image.

Happy you fixed!

I was still interested in discovering if the (string) cast was necessary. Probably nope. But who knows without testing, and I cannot reproduce unfortunately.

aklapper mentioned this in D25482: Fix exception in Transaction Log after renaming Pholio Mock image: Call to undefined method PholioImageSequenceTransaction::renderHandleLink().Nov 29 2023, 17:00