From fc0da499f83d028d4fab83a8df5cbf78fd171d6c Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Tue, 22 Jun 2021 14:07:23 -0400 Subject: [PATCH] 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 | 73 +++++++++-------- spec/components/svg_sprite/svg_sprite_spec.rb | 79 ++++++++++++++----- .../plugins/my_plugin/svg-icons/custom.svg | 10 +++ 3 files changed, 110 insertions(+), 52 deletions(-) create mode 100644 spec/fixtures/plugins/my_plugin/svg-icons/custom.svg diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb index c9eb7340c3d..2c0cbe69705 100644 --- a/lib/svg_sprite/svg_sprite.rb +++ b/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? - sources = sources + custom_svg_sprites(theme_id) + CORE_SVG_SPRITES.each do |path| + 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| - if !File.exist?(fname) - cache.delete("custom_svg_sprites_#{Theme.transform_ids(theme_id).join(',')}") - next - end - - svg_file = Nokogiri::XML(File.open(fname)) do |config| + custom_svg_sprites(theme_id).each do |item| + svg_file = Nokogiri::XML(item[:sprite]) 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| - next if !File.exist?(fname) - - svg_file = Nokogiri::XML(File.open(fname)) - svg_filename = "#{File.basename(fname, ".svg")}" + sprite_sources(SiteSetting.default_theme_id).each do |item| + svg_file = Nokogiri::XML(item[:sprite]) 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| - next if !File.exist?(fname) - - svg_file = Nokogiri::XML(File.open(fname)) - svg_filename = "#{File.basename(fname, ".svg")}" + sprite_sources(SiteSetting.default_theme_id).each do |item| + svg_file = Nokogiri::XML(item[:sprite]) 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| - next if !File.exist?(fname) - - svg_file = Nokogiri::XML(File.open(fname)) - + custom_svg_sprites(theme_id).each do |item| + svg_file = Nokogiri::XML(item[:sprite]) svg_file.css('symbol').each do |sym| icons << sym.attributes['id'].value if sym.attributes['id'].present? end diff --git a/spec/components/svg_sprite/svg_sprite_spec.rb b/spec/components/svg_sprite/svg_sprite_spec.rb index 584800ea028..0ade6bc3902 100644 --- a/spec/components/svg_sprite/svg_sprite_spec.rb +++ b/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).not_to match(/amazonaws/) + expect(sprite_files).to match(/my-custom-theme-icon/) 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 - expect(SvgSprite.cache.hash.keys).to_not include("custom_svg_sprites_#{theme.id}") - + # after a temp file is missing, bundling still works + 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 diff --git a/spec/fixtures/plugins/my_plugin/svg-icons/custom.svg b/spec/fixtures/plugins/my_plugin/svg-icons/custom.svg new file mode 100644 index 00000000000..742ac324e0d --- /dev/null +++ b/spec/fixtures/plugins/my_plugin/svg-icons/custom.svg @@ -0,0 +1,10 @@ + + + + + + + + + +