Page MenuHomePhabricator
Create Task
Maniphest T12614

Add file transform unit tests
Closed, ResolvedPublic
Actions

Description

We currently have no unit tests for the file transform functions, which can fail if no default fallback image is present. We should add some.

arcanist 146693307f607ffa93dfb99599f1989d5b27e03d (20 Apr 2017)
libphutil 6fe33623cda69b7814a0767866400e09f4eadf3a (13 Apr 2017)

After I upgraded Phabricator to the latest version and ran './bin/storage upgrade' command.
I get the following error.

Applying patch "phabricator:20161005.conpherence.image.2.php" to host "localhost:3306"...
[2017-04-21 13:54:19] EXCEPTION: (FilesystemException) File system entity '/opt/phabricator/apps/phabricator/htdocs/resources/builtin/image-400x400.png' does not exist. at [<phutil>/src/filesystem/Filesystem.php:1039]
arcanist(head=master, ref.master=146693307f60), phabricator(head=master, ref.master=ed9afa18cca5, custom=1), phutil(head=master, ref.master=6fe33623cda6)
#0 Filesystem::assertExists(string) called at [<phutil>/src/filesystem/Filesystem.php:37]
#1 Filesystem::readFile(string) called at [<phabricator>/src/applications/files/transform/PhabricatorFileThumbnailTransform.php:226]
#2 PhabricatorFileThumbnailTransform::getDefaultTransform(PhabricatorFile) called at [<phabricator>/src/applications/files/transform/PhabricatorFileTransform.php:27]
#3 PhabricatorFileTransform::executeTransform(PhabricatorFile) called at [<phabricator>/resources/sql/autopatches/20161005.conpherence.image.2.php:25]
#4 require_once(string) called at [<phabricator>/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php:285]
#5 PhabricatorStorageManagementAPI::applyPatchPHP(string) called at [<phabricator>/src/infrastructure/storage/management/PhabricatorStorageManagementAPI.php:241]
#6 PhabricatorStorageManagementAPI::applyPatch(PhabricatorStoragePatch) called at [<phabricator>/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php:1093]
#7 PhabricatorStorageManagementWorkflow::doUpgradeSchemata(array, NULL, boolean, boolean) called at [<phabricator>/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php:840]
#8 PhabricatorStorageManagementWorkflow::upgradeSchemata(array, NULL, boolean, boolean) called at [<phabricator>/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementUpgradeWorkflow.php:78]
#9 PhabricatorStorageManagementUpgradeWorkflow::didExecute(PhutilArgumentParser) called at [<phabricator>/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php:107]
#10 PhabricatorStorageManagementWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:441]
#11 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:333]
#12 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/sql/manage_storage.php:249]

Please help. Thanks.

Revisions and Commits

rP Phabricator
D17773rPbb2b91d28e7e Add unit tests for file thumbnail generation

Related Objects

Event Timeline

kevinchea created this task.Apr 21 2017, 7:05 AM
chad added a subscriber: chad.Apr 21 2017, 2:23 PM

How can we reproduce this locally?

epriestley added a subscriber: epriestley.EditedApr 21 2017, 2:29 PM

I suspect the reproduction steps are something like:

  1. Add profile images to Conpherence rooms prior to October, 2016 with gd installed.
  2. Uninstall the gd extension.
  3. Upgrade Phabricator to HEAD.

The steps might be slightly less silly, since step (2) could be "install gd for mod_php only, not for CLI PHP".

To work around this, install the gd extension, or enable it for CLI PHP if it is not yet enabled.

We'll fix this to work without gd. See also T12570.

chad added a comment.Apr 21 2017, 2:30 PM

I don't see a 400x400.png anywhere in builtin history.

epriestley added a comment.Apr 21 2017, 2:32 PM

I think the upstream actions are probably:

  • Add a unit test to make sure that all transforms can actually generate a default transform. This should tell us which builtin images are missing and prevent us from making this mistake again when we go up to 1600x1600.
  • Add the missing builtin images the test identifies.
chad added a comment.Apr 21 2017, 2:59 PM

Er, sorry, what mistake did we make again? I can’t find 400x400 ever
existing?

epriestley added a comment.Apr 21 2017, 3:05 PM

In D17295, we changed the "profile" transform to generate 400x400px images instead of 200x200px images. When we add or change a transform, we need to include a default result file for that transform which can be used as a fallback if the transform fails (most often because an install does not have gd, but possibly because of a corrupt image, inaccessible file storage, etc). That change didn't update the default result file, resources/builtin/image-200x200.png, so we were left with a 400x400px transform but a 200x200px unused default image that were not linked to one another.

(I think I previously made this mistake in D12808 when we upgraded to 100x100 although that's a little murky, and then we missed it again in D15097 when we upgraded to 200x200, although I later added the 200x200 in an unrelated change, D15171.)

chad added a comment.Apr 21 2017, 3:06 PM

oh i see... so I just make one for now?

epriestley added a comment.Apr 21 2017, 3:07 PM

Basically, we routinely double the size of transforms. When we do, we also need to replace default-111x111.png with default-222x222.png so we have a fallback. We have failed to do this ~every time we've doubled things since it's impossible to know/remember, so getting test coverage seems like a good idea.

epriestley added a comment.Apr 21 2017, 3:07 PM

Adding the file should fix this error, but we'll almost certainly break it again when we go to 800x800 without a test.

chad renamed this task from Error after upgrade Phabricator to Add file transform unit tests.Apr 21 2017, 5:43 PM
chad updated the task description. (Show Details)
epriestley claimed this task.Apr 23 2017, 1:02 PM
epriestley added a revision: D17773: Add unit tests for file thumbnail generation.Apr 23 2017, 1:10 PM
epriestley closed this task as Resolved by committing rPbb2b91d28e7e: Add unit tests for file thumbnail generation.Apr 23 2017, 6:02 PM
epriestley added a commit: rPbb2b91d28e7e: Add unit tests for file thumbnail generation.
Phabricator · Built by Phacility