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

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

diff --git a/app/controllers/svg_sprite_controller.rb b/app/controllers/svg_sprite_controller.rb
index 2a8e059..266db14 100644
--- a/app/controllers/svg_sprite_controller.rb
+++ b/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]
-        return redirect_to path(SvgSprite.path)
+      if SvgSprite.version(theme_ids) != params[:version]
+        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
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 4963b95..38b826b 100644
--- a/app/helpers/application_helper.rb
+++ b/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"]
diff --git a/app/models/theme.rb b/app/models/theme.rb
index 3b0be21..6a7854e 100644
--- a/app/models/theme.rb
+++ b/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?
 
diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb
index 34c24f1..740eaf4 100644
--- a/app/models/theme_field.rb
+++ b/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?
 
diff --git a/config/routes.rb b/config/routes.rb
index 955d0b1..dca765b 100644
--- a/config/routes.rb
+++ b/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\.-]+/ }
diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb
index 3717d59..217fa97 100644
--- a/lib/svg_sprite/svg_sprite.rb
+++ b/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
-    @svg_sprite_cache ||= DistributedCache.new('svg_sprite')
+  def self.all_icons(theme_ids = [])
+    get_set_cache("icons_#{Theme.transform_ids(theme_ids).join(',')}") do
+      Set.new()
+        .merge(settings_icons)
+        .merge(plugin_icons)
+        .merge(badge_icons)
+        .merge(group_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.all_icons
-    Set.new()
-      .merge(settings_icons)
-      .merge(plugin_icons)
-      .merge(badge_icons)
-      .merge(group_icons)
-      .merge(theme_icons)
-      .delete_if { |i| i.blank? || i.include?("/") }
-      .map! { |i| process(i.dup) }
-      .merge(SVG_ICONS)
-      .sort
+  def self.version(theme_ids = [])
+    get_set_cache("version_#{Theme.transform_ids(theme_ids).join(',')}") do
+      Digest::SHA1.hexdigest(all_icons(theme_ids).join('|'))
+    end
   end
 
-  def self.rebuild_cache
-    icons = all_icons
-    svg_sprite_cache['icons'] = icons
-    svg_sprite_cache['version'] = Digest::SHA1.hexdigest(icons.join('|'))
+  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
-  end
-
-  def self.version
-    svg_sprite_cache['version'] || rebuild_cache
+    cache&.clear
   end
 
-  def self.bundle
-    icons = svg_sprite_cache['icons'] || all_icons
+  def self.bundle(theme_ids = [])
+    icons = all_icons(theme_ids)
 
     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
-    Theme.all.each do |theme|
+    # Need to load full records for default values
+    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
diff --git a/spec/components/svg_sprite/svg_sprite_spec.rb b/spec/components/svg_sprite/svg_sprite_spec.rb
index 1d0fad9..b33fc70 100644
--- a/spec/components/svg_sprite/svg_sprite_spec.rb
+++ b/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")

[... diff too long, it was truncated ...]

GitHub sha: f3cfce4a