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.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 25 2018, 1:45 PM
epriestley requested review of this revision.Apr 25 2018, 1:47 PM
amckinley accepted this revision.Apr 25 2018, 4:39 PM
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.