Page MenuHomePhabricator

Recover more gracefully when favicon configuration points at a corrupt/damaged file
ClosedPublic

Authored by epriestley on Apr 25 2018, 1:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 7:13 AM
Unknown Object (File)
Mon, Apr 1, 11:28 PM
Unknown Object (File)
Fri, Mar 29, 3:17 PM
Unknown Object (File)
Wed, Mar 27, 7:44 AM
Unknown Object (File)
Mar 10 2024, 12:10 PM
Unknown Object (File)
Feb 6 2024, 4:16 PM
Unknown Object (File)
Feb 3 2024, 6:52 PM
Unknown Object (File)
Jan 25 2024, 12:45 AM
Subscribers
None

Details

Summary

Ref T13103. Locally, I managed to break the data for a bunch of files by doing git clean -df in a working copy that I'd updated to a commit from many many years ago. Since conf/local.json wasn't on the gitignore list many years ago, this removed it, and I lost my encryption keyring.

I've symlinked my local config to a version-controlled file now to avoid this specific type of creative self-sabotage in the future, but this has exposed a few cases where we could handle things more gracefully.

One issue is that if your favicon is customized but the file it points at can't actually be loaded, we fail explosively and you really can't do anything to move forward except somehow guess that you need to fix your favicon. Instead, recover more gracefully.

Test Plan
  • Configure file encryption.
  • Configure a favicon.
  • Remove the encryption key from your keyring.
  • Purge Phabricator's caches.
  • Before: you pretty much dead-end on a fatal that's hard to understand/fix.
  • After: everything works except your favicon.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/applications/files/favicon/PhabricatorFaviconRef.php
256

Maybe create a setup warning? I suppose this case is pretty rare, but it would otherwise be a mystery as to how to recover a deleted favicon.

This revision is now accepted and ready to land.Apr 25 2018, 4:39 PM

Maybe create a setup warning?

The best place for this is probably in the config option validator, but that only exists as a TODO in T13103 for now (the config option type is just "wild" right now, i.e. not validated). I added a note there about validating the file PHIDs and data specifically.

This revision was automatically updated to reflect the committed changes.