FEATURE: Calculate sprite-sheet based on currently active themes (#6973)

Previously there was only one sprite sheet, which always included icons from all themes even if they were disabled

app/controllers/svg_sprite_controller.rb

@@ -10,12 +10,13 @@ class SvgSpriteController < ApplicationController
     no_cookies
 
     RailsMultisite::ConnectionManagement.with_hostname(params[:hostname]) do
+      theme_ids = params[:theme_ids].split(",").map(&:to_i)
 
-      if SvgSprite.version != params[:version]
+      if SvgSprite.version(theme_ids) != params[:version]
-        return redirect_to path(SvgSprite.path)
+        return redirect_to path(SvgSprite.path(theme_ids))
       end
 
-      svg_sprite = "window.__svg_sprite = #{SvgSprite.bundle.inspect};"
+      svg_sprite = "window.__svg_sprite = #{SvgSprite.bundle(theme_ids).inspect};"
 
       response.headers["Last-Modified"] = 10.years.ago.httpdate
       response.headers["Content-Length"] = svg_sprite.bytesize.to_s

app/helpers/application_helper.rb

@@ -382,7 +382,7 @@ module ApplicationHelper
 
   def theme_ids
     if customization_disabled?
-      nil
+      [nil]
     else
       request.env[:resolved_theme_ids]
     end
@@ -454,11 +454,11 @@ module ApplicationHelper
       asset_version: Discourse.assets_digest,
       disable_custom_css: loading_admin?,
       highlight_js_path: HighlightJs.path,
-      svg_sprite_path: SvgSprite.path,
+      svg_sprite_path: SvgSprite.path(theme_ids),
     }
 
     if Rails.env.development?
-      setup_data[:svg_icon_list] = SvgSprite.all_icons
+      setup_data[:svg_icon_list] = SvgSprite.all_icons(theme_ids)
     end
 
     if guardian.can_enable_safe_mode? && params["safe_mode"]

app/models/theme.rb

@@ -121,6 +121,7 @@ class Theme < ActiveRecord::Base
   end
 
   def self.transform_ids(ids, extend: true)
+    return [] if ids.nil?
     get_set_cache "#{extend ? "extended_" : ""}transformed_ids_#{ids.join("_")}" do
       next [] if ids.blank?
 

app/models/theme_field.rb

@@ -7,6 +7,10 @@ class ThemeField < ActiveRecord::Base
   belongs_to :upload
   has_one :javascript_cache, dependent: :destroy
 
+  after_commit do |field|
+    SvgSprite.expire_cache if field.target_id == Theme.targets[:settings]
+  end
+
   scope :find_by_theme_ids, ->(theme_ids) {
     return none unless theme_ids.present?
 

config/routes.rb

@@ -453,7 +453,7 @@ Discourse::Application.routes.draw do
   # in most production settings this is bypassed
   get "letter_avatar_proxy/:version/letter/:letter/:color/:size.png" => "user_avatars#show_proxy_letter"
 
-  get "svg-sprite/:hostname/svg-:version.js" => "svg_sprite#show", format: false, constraints: { hostname: /[\w\.-]+/, version: /\h{40}/ }
+  get "svg-sprite/:hostname/svg-:theme_ids-:version.js" => "svg_sprite#show", format: false, constraints: { hostname: /[\w\.-]+/, version: /\h{40}/, theme_ids: /([0-9]+(,[0-9]+)*)?/ }
   get "svg-sprite/search/:keyword" => "svg_sprite#search", format: false, constraints: { keyword: /[-a-z0-9\s\%]+/ }
 
   get "highlight-js/:hostname/:version.js" => "highlight_js#show", format: false, constraints: { hostname: /[\w\.-]+/ }

lib/svg_sprite/svg_sprite.rb

@@ -188,39 +188,37 @@ module SvgSprite
   SVG_SPRITE_PATHS = Dir.glob(["#{Rails.root}/vendor/assets/svg-icons/**/*.svg",
                                "#{Rails.root}/plugins/*/svg-icons/*.svg"])
 
-  def self.svg_sprite_cache
+  def self.all_icons(theme_ids = [])
-    @svg_sprite_cache ||= DistributedCache.new('svg_sprite')
+    get_set_cache("icons_#{Theme.transform_ids(theme_ids).join(',')}") do
-  end
-
-  def self.all_icons
       Set.new()
         .merge(settings_icons)
         .merge(plugin_icons)
         .merge(badge_icons)
         .merge(group_icons)
-      .merge(theme_icons)
+        .merge(theme_icons(theme_ids))
         .delete_if { |i| i.blank? || i.include?("/") }
         .map! { |i| process(i.dup) }
         .merge(SVG_ICONS)
         .sort
     end
+  end
 
-  def self.rebuild_cache
+  def self.version(theme_ids = [])
-    icons = all_icons
+    get_set_cache("version_#{Theme.transform_ids(theme_ids).join(',')}") do
-    svg_sprite_cache['icons'] = icons
+      Digest::SHA1.hexdigest(all_icons(theme_ids).join('|'))
-    svg_sprite_cache['version'] = Digest::SHA1.hexdigest(icons.join('|'))
+    end
+  end
+
+  def self.path(theme_ids = [])
+    "/svg-sprite/#{Discourse.current_hostname}/svg-#{theme_ids&.join(",")}-#{version(theme_ids)}.js"
   end
 
   def self.expire_cache
-    svg_sprite_cache.clear
+    cache&.clear
   end
 
-  def self.version
+  def self.bundle(theme_ids = [])
-    svg_sprite_cache['version'] || rebuild_cache
+    icons = all_icons(theme_ids)
-  end
-
-  def self.bundle
-    icons = svg_sprite_cache['icons'] || all_icons
 
     doc = File.open("#{Rails.root}/vendor/assets/svg-icons/fontawesome/solid.svg") { |f| Nokogiri::XML(f) }
     fa_license = doc.at('//comment()').text
@@ -240,7 +238,6 @@ Discourse SVG subset of #{fa_license}
 
       svg_file.css('symbol').each do |sym|
         icon_id = prepare_symbol(sym, svg_filename)
-
         if icons.include? icon_id
           sym.attributes['id'].value = icon_id
           sym.css('title').each(&:remove)
@@ -286,10 +283,6 @@ Discourse SVG subset of #{fa_license}
     icon_id
   end
 
-  def self.path
-    "/svg-sprite/#{Discourse.current_hostname}/svg-#{version}.js"
-  end
-
   def self.settings_icons
     # includes svg_icon_subset and any settings containing _icon (incl. plugin settings)
     site_setting_icons = []
@@ -319,11 +312,11 @@ Discourse SVG subset of #{fa_license}
     Group.where("flair_url LIKE '%fa-%'").pluck(:flair_url).uniq
   end
 
-  def self.theme_icons
+  def self.theme_icons(theme_ids)
     theme_icon_settings = []
 
-    # Theme.all includes default values
+    # Need to load full records for default values
-    Theme.all.each do |theme|
+    Theme.where(id: Theme.transform_ids(theme_ids)).each do |theme|
       settings = theme.cached_settings.each do |key, value|
         if key.to_s.include?("_icon") && String === value
           theme_icon_settings |= value.split('|')
@@ -347,4 +340,12 @@ Discourse SVG subset of #{fa_license}
     FA_ICON_MAP.each { |k, v| icon_name.sub!(k, v) }
     fa4_to_fa5_names[icon_name] || icon_name
   end
+
+  def self.get_set_cache(key)
+    cache[key] ||= yield
+  end
+
+  def self.cache
+    @cache ||= DistributedCache.new('svg_sprite')
+  end
 end

spec/components/svg_sprite/svg_sprite_spec.rb

@@ -3,7 +3,7 @@ require 'rails_helper'
 describe SvgSprite do
 
   before do
-    SvgSprite.rebuild_cache
+    SvgSprite.expire_cache
   end
 
   it 'can generate a bundle' do
@@ -12,6 +12,15 @@ describe SvgSprite do
     expect(bundle).to match(/angle-double-down/)
   end
 
+  it 'can generate paths' do
+    version = SvgSprite.version # Icons won't change for this test
+    expect(SvgSprite.path).to eq("/svg-sprite/#{Discourse.current_hostname}/svg--#{version}.js")
+    expect(SvgSprite.path([1, 2])).to eq("/svg-sprite/#{Discourse.current_hostname}/svg-1,2-#{version}.js")
+
+    # Safe mode
+    expect(SvgSprite.path([nil])).to eq("/svg-sprite/#{Discourse.current_hostname}/svg--#{version}.js")
+  end
+
   it 'can search for a specific FA icon' do
     expect(SvgSprite.search("fa-heart")).to match(/heart/)
     expect(SvgSprite.search("poo-storm")).to match(/poo-storm/)
@@ -51,26 +60,43 @@ describe SvgSprite do
 
   it 'includes icons defined in theme settings' do
     theme = Fabricate(:theme)
-    theme.set_field(target: :settings, name: :yaml, value: "custom_icon: magic")
+
+    # Works for default settings:
+    theme.set_field(target: :settings, name: :yaml, value: "custom_icon: dragon")
     theme.save!
+    expect(SvgSprite.all_icons([theme.id])).to include("dragon")
 
-    # TODO: add test for default settings values
+    # Automatically purges cache when default changes:
+    theme.set_field(target: :settings, name: :yaml, value: "custom_icon: gamepad")
+    theme.save!
+    expect(SvgSprite.all_icons([theme.id])).to include("gamepad")
 
+    # Works when applying override
     theme.update_setting(:custom_icon, "gas-pump")
-    expect(SvgSprite.all_icons).to include("gas-pump")
+    expect(SvgSprite.all_icons([theme.id])).to include("gas-pump")
 
+    # Works when changing override
     theme.update_setting(:custom_icon, "gamepad")
-    expect(SvgSprite.all_icons).to include("gamepad")
+    expect(SvgSprite.all_icons([theme.id])).to include("gamepad")
-    expect(SvgSprite.all_icons).not_to include("gas-pump")
+    expect(SvgSprite.all_icons([theme.id])).not_to include("gas-pump")
 
     # FA5 syntax
     theme.update_setting(:custom_icon, "fab fa-bandcamp")
-    expect(SvgSprite.all_icons).to include("fab-bandcamp")
+    expect(SvgSprite.all_icons([theme.id])).to include("fab-bandcamp")
 
     # Internal Discourse syntax + multiple icons
     theme.update_setting(:custom_icon, "fab-android|dragon")
-    expect(SvgSprite.all_icons).to include("fab-android")
+    expect(SvgSprite.all_icons([theme.id])).to include("fab-android")
-    expect(SvgSprite.all_icons).to include("dragon")
+    expect(SvgSprite.all_icons([theme.id])).to include("dragon")
+
+    # Check themes don't leak into non-theme sprite sheet
+    expect(SvgSprite.all_icons).not_to include("dragon")
+
+    # Check components are included
+    theme.update(component: true)
+    parent_theme = Fabricate(:theme)
+    parent_theme.add_child_theme!(theme)
+    expect(SvgSprite.all_icons([parent_theme.id])).to include("dragon")
   end
 
   it 'includes icons from SiteSettings' do
@@ -82,10 +108,12 @@ describe SvgSprite do
     expect(all_icons).to include("fab-bandcamp")
 
     SiteSetting.svg_icon_subset = nil
+    SvgSprite.expire_cache
     expect(SvgSprite.all_icons).not_to include("drafting-compass")
 
     # does not fail on non-string setting
     SiteSetting.svg_icon_subset = false
+    SvgSprite.expire_cache
     expect(SvgSprite.all_icons).to be_truthy
   end
 

spec/requests/svg_sprite_controller_spec.rb

@@ -4,17 +4,23 @@ describe SvgSpriteController do
 
   context 'show' do
     before do
-      SvgSprite.rebuild_cache
+      SvgSprite.expire_cache
     end
 
     it "should return bundle when version is current" do
-      get "/svg-sprite/#{Discourse.current_hostname}/svg-#{SvgSprite.version}.js"
+      get "/svg-sprite/#{Discourse.current_hostname}/svg--#{SvgSprite.version}.js"
+      expect(response.status).to eq(200)
+
+      theme = Fabricate(:theme)
+      theme.set_field(target: :settings, name: :yaml, value: "custom_icon: dragon")
+      theme.save!
+      get "/svg-sprite/#{Discourse.current_hostname}/svg-#{theme.id}-#{SvgSprite.version([theme.id])}.js"
       expect(response.status).to eq(200)
     end
 
     it "should redirect to current version" do
       random_hash = Digest::SHA1.hexdigest("somerandomstring")
-      get "/svg-sprite/#{Discourse.current_hostname}/svg-#{random_hash}.js"
+      get "/svg-sprite/#{Discourse.current_hostname}/svg--#{random_hash}.js"
 
       expect(response.status).to eq(302)
       expect(response.location).to include(SvgSprite.version)