DEV: Refactor custom svg icon caching (#13483)

Previously, we were storing custom svg sprite paths in the cache. This is a problem because sprites in themes get stored as uploads, and the returned paths were files in the temporary download cache which could sometimes be cleaned up, resulting in a broken cache.

I previously tried to fix this by skipping the missing files and clearing the cache, but that didn't work out well with CDNs. This PR stores the contents of the files in the custom_svg_sprites cache to avoid the problem of missing temp files.

Also, plugin custom icons are only included if the plugin is enabled.

lib/svg_sprite/svg_sprite.rb

@@ -230,7 +230,12 @@ module SvgSprite
 
   def self.custom_svg_sprites(theme_id)
     get_set_cache("custom_svg_sprites_#{Theme.transform_ids(theme_id).join(',')}") do
-      custom_sprite_paths = Dir.glob("#{Rails.root}/plugins/*/svg-icons/*.svg")
+      plugin_paths = []
+      Discourse.plugins.map { |plugin| File.dirname(plugin.path) }.each do |path|
+        plugin_paths << "#{path}/svg-icons/*.svg"
+      end
+
+      custom_sprite_paths = Dir.glob(plugin_paths)
 
       if theme_id.present?
         ThemeField.where(type_id: ThemeField.types[:theme_upload_var], name: THEME_SPRITE_VAR_NAME, theme_id: Theme.transform_ids(theme_id))
@@ -249,7 +254,14 @@ module SvgSprite
         end
       end
 
-      custom_sprite_paths
+      custom_sprite_paths.map do |path|
+        if File.exist?(path)
+          {
+            filename: "#{File.basename(path, ".svg")}",
+            sprite: File.read(path)
+          }
+        end
+      end
     end
   end
 
@@ -284,13 +296,22 @@ module SvgSprite
   end
 
   def self.sprite_sources(theme_id)
-    sources = CORE_SVG_SPRITES
+    sprites = []
 
-    if theme_id.present?
+    CORE_SVG_SPRITES.each do |path|
-      sources = sources + custom_svg_sprites(theme_id)
+      if File.exist?(path)
+        sprites << {
+          filename: "#{File.basename(path, ".svg")}",
+          sprite: File.read(path)
+        }
+      end
     end
 
-    sources
+    if theme_id.present?
+      sprites = sprites + custom_svg_sprites(theme_id)
+    end
+
+    sprites
   end
 
   def self.core_svgs
@@ -329,20 +350,13 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
       end
     end
 
-    custom_svg_sprites(theme_id).each do |fname|
+    custom_svg_sprites(theme_id).each do |item|
-      if !File.exist?(fname)
+      svg_file = Nokogiri::XML(item[:sprite]) do |config|
-        cache.delete("custom_svg_sprites_#{Theme.transform_ids(theme_id).join(',')}")
-        next
-      end
-
-      svg_file = Nokogiri::XML(File.open(fname)) do |config|
         config.options = Nokogiri::XML::ParseOptions::NOBLANKS
       end
 
-      svg_filename = "#{File.basename(fname, ".svg")}"
-
       svg_file.css("symbol").each do |sym|
-        icon_id = prepare_symbol(sym, svg_filename)
+        icon_id = prepare_symbol(sym, item[:filename])
 
         if icons.include? icon_id
           sym.attributes['id'].value = icon_id
@@ -358,14 +372,11 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
   def self.search(searched_icon)
     searched_icon = process(searched_icon.dup)
 
-    sprite_sources(SiteSetting.default_theme_id).each do |fname|
+    sprite_sources(SiteSetting.default_theme_id).each do |item|
-      next if !File.exist?(fname)
+      svg_file = Nokogiri::XML(item[:sprite])
-
-      svg_file = Nokogiri::XML(File.open(fname))
-      svg_filename = "#{File.basename(fname, ".svg")}"
 
       svg_file.css('symbol').each do |sym|
-        icon_id = prepare_symbol(sym, svg_filename)
+        icon_id = prepare_symbol(sym, item[:filename])
 
         if searched_icon == icon_id
           sym.attributes['id'].value = icon_id
@@ -381,14 +392,11 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
   def self.icon_picker_search(keyword)
     results = Set.new
 
-    sprite_sources(SiteSetting.default_theme_id).each do |fname|
+    sprite_sources(SiteSetting.default_theme_id).each do |item|
-      next if !File.exist?(fname)
+      svg_file = Nokogiri::XML(item[:sprite])
-
-      svg_file = Nokogiri::XML(File.open(fname))
-      svg_filename = "#{File.basename(fname, ".svg")}"
 
       svg_file.css('symbol').each do |sym|
-        icon_id = prepare_symbol(sym, svg_filename)
+        icon_id = prepare_symbol(sym, item[:filename])
         if keyword.empty? || icon_id.include?(keyword)
           sym.attributes['id'].value = icon_id
           sym.css('title').each(&:remove)
@@ -417,7 +425,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
     THEME_SPRITE_VAR_NAME
   end
 
-  def self.prepare_symbol(symbol, svg_filename)
+  def self.prepare_symbol(symbol, svg_filename = nil)
     icon_id = symbol.attr('id')
 
     case svg_filename
@@ -484,11 +492,8 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
   def self.custom_icons(theme_id)
     # Automatically register icons in sprites added via themes or plugins
     icons = []
-    custom_svg_sprites(theme_id).each do |fname|
+    custom_svg_sprites(theme_id).each do |item|
-      next if !File.exist?(fname)
+      svg_file = Nokogiri::XML(item[:sprite])
-
-      svg_file = Nokogiri::XML(File.open(fname))
-
       svg_file.css('symbol').each do |sym|
         icons << sym.attributes['id'].value if sym.attributes['id'].present?
       end

spec/components/svg_sprite/svg_sprite_spec.rb

@@ -156,19 +156,6 @@ describe SvgSprite do
     expect(icons).to include("fly")
   end
 
-  it 'includes custom icons from a sprite in a theme' do
-    theme = Fabricate(:theme)
-    fname = "custom-theme-icon-sprite.svg"
-
-    upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1)
-
-    theme.set_field(target: :common, name: SvgSprite.theme_sprite_variable_name, upload_id: upload.id, type: :theme_upload_var)
-    theme.save!
-
-    expect(Upload.where(id: upload.id)).to be_exist
-    expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/)
-  end
-
   context "s3" do
     let(:upload_s3) { Fabricate(:upload_s3) }
 
@@ -191,8 +178,7 @@ describe SvgSprite do
       theme.save!
 
       sprite_files = SvgSprite.custom_svg_sprites(theme.id).join("|")
-      expect(sprite_files).to match(/#{upload_s3.sha1}/)
+      expect(sprite_files).to match(/my-custom-theme-icon/)
-      expect(sprite_files).not_to match(/amazonaws/)
 
       SvgSprite.bundle(theme.id)
       expect(SvgSprite.cache.hash.keys).to include("custom_svg_sprites_#{theme.id}")
@@ -201,9 +187,8 @@ describe SvgSprite do
       File.delete external_copy.try(:path)
 
       SvgSprite.bundle(theme.id)
-      # when a file is missing, ensure that cache entry is cleared
+      # after a temp file is missing, bundling still works
-      expect(SvgSprite.cache.hash.keys).to_not include("custom_svg_sprites_#{theme.id}")
+      expect(SvgSprite.cache.hash.keys).to include("custom_svg_sprites_#{theme.id}")
-
     end
   end
 
@@ -237,4 +222,62 @@ describe SvgSprite do
     group = Fabricate(:group, flair_icon: "far-building")
     expect(SvgSprite.bundle).to match(/far-building/)
   end
+
+  describe "#custom_svg_sprites" do
+    it 'is empty by default' do
+      expect(SvgSprite.custom_svg_sprites(nil)).to be_empty
+      expect(SvgSprite.bundle).not_to be_empty
+    end
+
+    context "with a plugin" do
+      let :plugin1 do
+        plugin1 = Plugin::Instance.new
+        plugin1.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb"
+        plugin1
+      end
+
+      before do
+        Discourse.plugins << plugin1
+        plugin1.activate!
+      end
+
+      after do
+        Discourse.plugins.delete plugin1
+      end
+
+      it "includes custom icons from plugins" do
+        expect(SvgSprite.custom_svg_sprites(nil).size).to eq(1)
+        expect(SvgSprite.bundle).to match(/custom-icon/)
+      end
+    end
+
+    it 'includes custom icons in a theme' do
+      theme = Fabricate(:theme)
+      fname = "custom-theme-icon-sprite.svg"
+
+      upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1)
+
+      theme.set_field(target: :common, name: SvgSprite.theme_sprite_variable_name, upload_id: upload.id, type: :theme_upload_var)
+      theme.save!
+
+      expect(Upload.where(id: upload.id)).to be_exist
+      expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/)
+    end
+
+    it 'includes custom icons in a child theme' do
+      theme = Fabricate(:theme)
+      fname = "custom-theme-icon-sprite.svg"
+      child_theme = Fabricate(:theme, component: true)
+      theme.add_relative_theme!(:child, child_theme)
+
+      upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1)
+
+      child_theme.set_field(target: :common, name: SvgSprite.theme_sprite_variable_name, upload_id: upload.id, type: :theme_upload_var)
+      child_theme.save!
+
+      expect(Upload.where(id: upload.id)).to be_exist
+      expect(SvgSprite.bundle(theme.id)).to match(/my-custom-theme-icon/)
+    end
+
+  end
 end

spec/fixtures/plugins/my_plugin/svg-icons/custom.svg

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="utf-8"?>
+<svg xmlns="http://www.w3.org/2000/svg">
+  <symbol id="custom-icon" viewBox="0 0 512 512">
+    <g>
+      <g>
+        <path d="M425.199,223.957c-13.303-13.303-34.961-13.303-48.205-0.06l-86.861,85.086V34.133C290.133,15.309,274.824,0,256,0"/>
+      </g>
+    </g>
+  </symbol>
+</svg>