Avoid exception setting project profile image when GD not installed
ClosedPublic
Actions

Authored by aklapper on Jan 13 2024, 20:54.

Details

Reviewers

avivey
valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15326: PHP 8.0 without GD installed: Unhandled Exception setting a project profile image: Call to undefined function imagecreatefromstring()

Commits

rP7a86040f8a46: Avoid exception setting project profile image when GD not installed

Summary

When trying to set a custom project profile image while the PHP GD extension is not installed, use the same logic which already exists in PhabricatorFilesComposeAvatarBuiltinFile.php to set the default project image. This stills display an unhelpful error message This server only supports these image formats: . but avoids an exception trying to call GD's imagecreatefromstring().

EXCEPTION: (Error) Call to undefined function imagecreatefromstring() at [<phorge>/src/applications/files/builtin/PhabricatorFilesComposeIconBuiltinFile.php:131]

Closes T15326

Test Plan

Remove the php-gd (and potentially gd) packages on your system; restart httpd
Go to http://phorge.localhost/project/manage/1/
Select Edit Picture in the sidebar on the right to go to the Edit Project Picture at http://phorge.localhost/project/picture/1/
Set a custom icon and color and click the Save Image button
Get This server only supports these image formats: . but no exception anymore

Diff Detail

Repository

rP Phorge

Branch

T15326gdException (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1021
Build 1021: arc lint + arc unit

Event Timeline

aklapper created this revision.Jan 13 2024, 20:54

Owners added a reviewer: O1: Blessed Committers.Jan 13 2024, 20:54

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 13 2024, 20:54

aklapper requested review of this revision.Jan 13 2024, 20:54

Harbormaster completed remote builds in B1021: Diff 1636.Jan 13 2024, 20:55

Do we have a setup-warning about the gd extension being missing?

This revision is now accepted and ready to land.Jan 14 2024, 08:32

In D25515#14801, @avivey wrote:

Do we have a setup-warning about the gd extension being missing?

@avivey: In theory yes in https://we.phorge.it/source/phorge/browse/master/src/applications/config/check/PhabricatorGDSetupCheck.php .
In practice I have no idea how to trigger it (or how/when these setup checks are performed) for an existing Phorge instance that I'm not about to freshly set up.

I wonder if it then, upon deletion of the profile image, it attempts to delete the builtin image

In D25515#14842, @valerio.bozzolan wrote:

I wonder if it then, upon deletion of the profile image, it attempts to delete the builtin image

How to delete a profile image? Steps welcome so I could test.

I was able to trigger the "missing gd" warning (By uninstalling php*gd), so we're good there.
A bit puzzled about how you got the error if you already have it installed?

The only way to delete things is using ./bin/remove destroy Fxxx from the command line.
For built-ins, I think it might attempt to delete them (They have an File object), and then they will be re-created automatically by something.

In D25515#14842, @valerio.bozzolan wrote:

I wonder if it then, upon deletion of the profile image, it attempts to delete the builtin image

@valerio.bozzolan How is attempting to delete a built-in (!) profile image related to this patch? If I misunderstood, please elaborate. Thanks!

In D25515#14957, @avivey wrote:

I was able to trigger the "missing gd" warning (By uninstalling php*gd), so we're good there.
A bit puzzled about how you got the error if you already have it installed?

Sorry, should have been more verbose. I uninstalled both php-gd and gd on a Fedora 39 system to trigger this. (Somehow also the latter was needed, the former didn't suffice.)

In D25515#14983, @aklapper wrote:

In D25515#14842, @valerio.bozzolan wrote:

I wonder if it then, upon deletion of the profile image, it attempts to delete the builtin image

@valerio.bozzolan How is attempting to delete a built-in (!) profile image related to this patch? If I misunderstood, please elaborate. Thanks!

AFAIK the method composeImage() was supposed to create something new (so, probably writable and deletable). Now instead it can return something that is supposed to be read-only.

I also can't think of a way to delete a profile picture either, so probably my note means nothing.

(By the way the Filesystem::readFile() does not return a pointer to a (potentially read-only) resource, but it just returns its content - so the concern was nonsense)

https://we.phorge.it/source/arcanist/browse/master/src/filesystem/Filesystem.php$41-48

Closed by commit rP7a86040f8a46: Avoid exception setting project profile image when GD not installed. · Explain WhyJan 23 2024, 14:11

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rP7a86040f8a46: Avoid exception setting project profile image when GD not installed.