From 1f6ad5e7ddd1d631d4cdccdc874c234b21908ee3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 20 Oct 2016 11:14:33 -0700 Subject: [PATCH] Support ".ico" in Celerity and simplify rewite rule configuration Summary: See D16734. - Add ".ico" files to the Celerity map. - Add a formal route for "/favicon.ico". - Remove instructions to configure `/rsrc/` and `/favicon.ico` rewrite rules. Long ago, we served resources directly via `/rsrc/` in at least some cases. As we added more features, this stopped working more and more often (for example, Apache can never serve CSS this way, because it doesn't know how to post-process `{$variables}`). In modern code (until this change), only `/favicon.ico` is still expected to be served this way. Instead, serve it with an explicit route via controller (this allows different Sites to have different favicons, for example). Remove the instructions suggesting the old rewrite rules be configured. It's OK if they're still in place -- they won't break anything, so we don't need to rush to get users to delete them. We should keep "webroot/favicon.ico" in place for now, since it needs to be there for users with the old rewrite rule. Test Plan: - Ran celerity map. - Loaded `/favicon.ico`, got resource via route. - Used `celerity_generate_resource_uri()` to get paths to other icons, loaded them, got icons. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D16737 --- resources/celerity/map.php | 5 +++++ src/__phutil_library_map__.php | 2 ++ .../controller/CelerityResourceController.php | 1 + .../resources/CelerityResourcesOnDisk.php | 1 + .../PhabricatorSystemApplication.php | 1 + .../PhabricatorSystemFaviconController.php | 19 +++++++++++++++++++ .../configuration/configuration_guide.diviner | 8 -------- 7 files changed, 29 insertions(+), 8 deletions(-) create mode 100644 src/applications/system/controller/PhabricatorSystemFaviconController.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 350c5c53b0..96181688f8 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -16,6 +16,7 @@ return array( 'differential.pkg.js' => '634399e9', 'diffusion.pkg.css' => '91c5d3a6', 'diffusion.pkg.js' => '84c8f8fd', + 'favicon.ico' => '30672e08', 'maniphest.pkg.css' => '4845691a', 'maniphest.pkg.js' => '949a7498', 'rsrc/css/aphront/aphront-bars.css' => '231ac33c', @@ -285,6 +286,7 @@ return array( 'rsrc/favicons/dark/favicon-196x196.png' => '5e06ee72', 'rsrc/favicons/dark/favicon-32x32.png' => 'bdd7e16b', 'rsrc/favicons/dark/favicon-96x96.png' => '0cf55978', + 'rsrc/favicons/dark/favicon.ico' => '4343aaa6', 'rsrc/favicons/dark/mstile-144x144.png' => '4dc9d42d', 'rsrc/favicons/dark/mstile-150x150.png' => '2dc61c90', 'rsrc/favicons/dark/mstile-310x150.png' => '4fe58ab2', @@ -295,6 +297,7 @@ return array( 'rsrc/favicons/favicon-196x196.png' => '95db275e', 'rsrc/favicons/favicon-32x32.png' => '5bd18b6c', 'rsrc/favicons/favicon-96x96.png' => '7242c8e9', + 'rsrc/favicons/favicon.ico' => 'cdb11121', 'rsrc/favicons/mask-icon.svg' => 'e132a80f', 'rsrc/favicons/mstile-144x144.png' => '310c2ee5', 'rsrc/favicons/mstile-150x150.png' => '74bf5133', @@ -314,6 +317,7 @@ return array( 'rsrc/favicons/red/favicon-196x196.png' => '94c089a5', 'rsrc/favicons/red/favicon-32x32.png' => '5848673e', 'rsrc/favicons/red/favicon-96x96.png' => '895d54e8', + 'rsrc/favicons/red/favicon.ico' => '25172b6b', 'rsrc/favicons/red/mstile-144x144.png' => '448639f5', 'rsrc/favicons/red/mstile-150x150.png' => 'c2ba45d0', 'rsrc/favicons/red/mstile-310x150.png' => 'b0e50799', @@ -332,6 +336,7 @@ return array( 'rsrc/favicons/yellow/favicon-196x196.png' => '932c7c65', 'rsrc/favicons/yellow/favicon-32x32.png' => '005c9f92', 'rsrc/favicons/yellow/favicon-96x96.png' => '3ad9bfa9', + 'rsrc/favicons/yellow/favicon.ico' => '2f5b2991', 'rsrc/favicons/yellow/mstile-144x144.png' => 'fc52335c', 'rsrc/favicons/yellow/mstile-150x150.png' => '9e556f80', 'rsrc/favicons/yellow/mstile-310x150.png' => '2c915073', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e859dcdce9..eafb0acdd5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3808,6 +3808,7 @@ phutil_register_library_map(array( 'PhabricatorSystemDAO' => 'applications/system/storage/PhabricatorSystemDAO.php', 'PhabricatorSystemDestructionGarbageCollector' => 'applications/system/garbagecollector/PhabricatorSystemDestructionGarbageCollector.php', 'PhabricatorSystemDestructionLog' => 'applications/system/storage/PhabricatorSystemDestructionLog.php', + 'PhabricatorSystemFaviconController' => 'applications/system/controller/PhabricatorSystemFaviconController.php', 'PhabricatorSystemReadOnlyController' => 'applications/system/controller/PhabricatorSystemReadOnlyController.php', 'PhabricatorSystemRemoveDestroyWorkflow' => 'applications/system/management/PhabricatorSystemRemoveDestroyWorkflow.php', 'PhabricatorSystemRemoveLogWorkflow' => 'applications/system/management/PhabricatorSystemRemoveLogWorkflow.php', @@ -8942,6 +8943,7 @@ phutil_register_library_map(array( 'PhabricatorSystemDAO' => 'PhabricatorLiskDAO', 'PhabricatorSystemDestructionGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorSystemDestructionLog' => 'PhabricatorSystemDAO', + 'PhabricatorSystemFaviconController' => 'PhabricatorController', 'PhabricatorSystemReadOnlyController' => 'PhabricatorController', 'PhabricatorSystemRemoveDestroyWorkflow' => 'PhabricatorSystemRemoveWorkflow', 'PhabricatorSystemRemoveLogWorkflow' => 'PhabricatorSystemRemoveWorkflow', diff --git a/src/applications/celerity/controller/CelerityResourceController.php b/src/applications/celerity/controller/CelerityResourceController.php index c1ea652084..95d674c641 100644 --- a/src/applications/celerity/controller/CelerityResourceController.php +++ b/src/applications/celerity/controller/CelerityResourceController.php @@ -145,6 +145,7 @@ abstract class CelerityResourceController extends PhabricatorController { 'eot' => 'font/eot', 'ttf' => 'font/ttf', 'mp3' => 'audio/mpeg', + 'ico' => 'image/x-icon', ); } diff --git a/src/applications/celerity/resources/CelerityResourcesOnDisk.php b/src/applications/celerity/resources/CelerityResourcesOnDisk.php index 3187c5b9ad..1f9e614da9 100644 --- a/src/applications/celerity/resources/CelerityResourcesOnDisk.php +++ b/src/applications/celerity/resources/CelerityResourcesOnDisk.php @@ -39,6 +39,7 @@ abstract class CelerityResourcesOnDisk extends CelerityPhysicalResources { 'ttf', 'eot', 'mp3', + 'ico', ); } diff --git a/src/applications/system/application/PhabricatorSystemApplication.php b/src/applications/system/application/PhabricatorSystemApplication.php index 0ec8f6a7a4..7ce2b4dbe0 100644 --- a/src/applications/system/application/PhabricatorSystemApplication.php +++ b/src/applications/system/application/PhabricatorSystemApplication.php @@ -26,6 +26,7 @@ final class PhabricatorSystemApplication extends PhabricatorApplication { '/readonly/' => array( '(?P[^/]+)/' => 'PhabricatorSystemReadOnlyController', ), + '/favicon.ico' => 'PhabricatorSystemFaviconController', ); } diff --git a/src/applications/system/controller/PhabricatorSystemFaviconController.php b/src/applications/system/controller/PhabricatorSystemFaviconController.php new file mode 100644 index 0000000000..d9a31cbcb6 --- /dev/null +++ b/src/applications/system/controller/PhabricatorSystemFaviconController.php @@ -0,0 +1,19 @@ +setContent($content) + ->setMimeType('image/x-icon') + ->setCacheDurationInSeconds(phutil_units('24 hours in seconds')) + ->setCanCDN(true); + } +} diff --git a/src/docs/user/configuration/configuration_guide.diviner b/src/docs/user/configuration/configuration_guide.diviner index b420996848..b4c494449b 100644 --- a/src/docs/user/configuration/configuration_guide.diviner +++ b/src/docs/user/configuration/configuration_guide.diviner @@ -48,8 +48,6 @@ this: DocumentRoot /path/to/phabricator/webroot RewriteEngine on - RewriteRule ^/rsrc/(.*) - [L,QSA] - RewriteRule ^/favicon.ico - [L,QSA] RewriteRule ^(.*)$ /index.php?__path__=$1 [B,L,QSA] @@ -91,10 +89,6 @@ For nginx, use a configuration like this: rewrite ^/(.*)$ /index.php?__path__=/$1 last; } - location = /favicon.ico { - try_files $uri =204; - } - location /index.php { fastcgi_pass localhost:9000; fastcgi_index index.php; @@ -130,8 +124,6 @@ For lighttpd, add a section like this to your lighttpd.conf: $HTTP["host"] =~ "phabricator(\.example\.com)?" { server.document-root = "/path/to/phabricator/webroot" url.rewrite-once = ( - "^(/rsrc/.*)$" => "$1", - "^(/favicon.ico)$" => "$1", # This simulates QSA ("query string append") mode in apache "^(/[^?]*)\?(.*)" => "/index.php?__path__=$1&$2", "^(/.*)$" => "/index.php?__path__=$1",