Code review comments.

app/assets/javascripts/discourse/app/lib/theme-selector.js

@@ -2,7 +2,7 @@ import cookie, { removeCookie } from "discourse/lib/cookie";
 import I18n from "I18n";
 import deprecated from "discourse-common/lib/deprecated";
 
-const keySelector = "meta[name=discourse_theme_ids]";
+const keySelector = "meta[name=discourse_theme_id]";
 
 export function currentThemeKey() {
   // eslint-disable-next-line no-console

app/assets/javascripts/discourse/lib/bootstrap-json/index.js

@@ -34,9 +34,9 @@ function head(buffer, bootstrap) {
     buffer.push(`<meta name="csrf-param" content="authenticity_token">`);
     buffer.push(`<meta name="csrf-token" content="${bootstrap.csrf_token}">`);
   }
-  if (bootstrap.theme_ids) {
+  if (bootstrap.theme_id) {
     buffer.push(
-      `<meta name="discourse_theme_ids" content="${bootstrap.theme_ids}">`
+      `<meta name="discourse_theme_id" content="${bootstrap.theme_id}">`
     );
   }
 

app/assets/javascripts/discourse/tests/acceptance/user-preferences-interface-test.js

@@ -100,7 +100,7 @@ acceptance("User Preferences - Interface", function (needs) {
 
   test("shows no default option for light scheme when theme's color scheme is user selectable", async function (assert) {
     let meta = document.createElement("meta");
-    meta.name = "discourse_theme_ids";
+    meta.name = "discourse_theme_id";
     meta.content = "2";
     document.getElementsByTagName("head")[0].appendChild(meta);
 
@@ -128,7 +128,7 @@ acceptance("User Preferences - Interface", function (needs) {
     await selectKit(".light-color-scheme .select-kit").expand();
     assert.equal(count(".light-color-scheme .select-kit .select-kit-row"), 2);
 
-    document.querySelector("meta[name='discourse_theme_ids']").remove();
+    document.querySelector("meta[name='discourse_theme_id']").remove();
   });
 });
 

app/controllers/bootstrap_controller.rb

@@ -51,7 +51,7 @@ class BootstrapController < ApplicationController
     ).map { |f| script_asset_path(f) }
 
     bootstrap = {
-      theme_ids: [theme_id],
+      theme_id: theme_id,
       title: SiteSetting.title,
       current_homepage: current_homepage,
       locale_script: locale,

app/controllers/stylesheets_controller.rb

@@ -47,7 +47,7 @@ class StylesheetsController < ApplicationController
         theme_id =
           if target.include?("theme")
             split_target, theme_id = target.split(/_(-?[0-9]+)/)
-            Theme.where(id: theme_id).pluck_first(:id) if theme_id.present?
+            theme_id if theme_id.present? && Theme.exists?(id: theme_id)
           else
             split_target, color_scheme_id = target.split(/_(-?[0-9]+)/)
             Theme.where(color_scheme_id: color_scheme_id).pluck_first(:id)

app/models/theme.rb

@@ -203,6 +203,7 @@ class Theme < ActiveRecord::Base
 
   def self.transform_ids(id)
     return [] if id.blank?
+    id = id.to_i
 
     get_set_cache "transformed_ids_#{id}" do
       all_ids =
@@ -297,8 +298,7 @@ class Theme < ActiveRecord::Base
   end
 
   def self.lookup_modifier(theme_ids, modifier_name)
-    theme_ids = [theme_ids] unless Array === theme_ids
+    theme_ids = [theme_ids] unless theme_ids.is_a?(Array)
-    theme_ids = transform_ids(theme_ids)
 
     get_set_cache("#{theme_ids.join(",")}:modifier:#{modifier_name}:#{Theme.compiler_version}") do
       ThemeModifierSet.resolve_modifier_for_themes(theme_ids, modifier_name)
@@ -350,7 +350,7 @@ class Theme < ActiveRecord::Base
   end
 
   def self.refresh_message_for_targets(targets, theme_ids)
-    theme_ids = [theme_ids] unless theme_ids === Array
+    theme_ids = [theme_ids] unless theme_ids.is_a?(Array)
 
     targets.each_with_object([]) do |target, data|
       theme_ids.each do |theme_id|

app/views/common/_discourse_publish_stylesheet.html.erb

@@ -1,4 +1,4 @@
-<%= discourse_stylesheet_link_tag 'publish', theme_ids: nil %>
+<%= discourse_stylesheet_link_tag 'publish', theme_id: nil %>
 
 <%- if rtl? %>
   <%= discourse_stylesheet_link_tag(mobile_view? ? :publish_mobile_rtl : :publish_mobile_rtl) %>

app/views/layouts/application.html.erb

@@ -5,7 +5,7 @@
     <meta charset="utf-8">
     <title><%= content_for?(:title) ? yield(:title) : SiteSetting.title %></title>
     <meta name="description" content="<%= @description_meta || SiteSetting.site_description %>">
-    <meta name="discourse_theme_ids" content="<%= theme_id %>">
+    <meta name="discourse_theme_id" content="<%= theme_id %>">
     <meta name="discourse_current_homepage" content="<%= current_homepage %>">
     <%= render partial: "layouts/head" %>
     <%= discourse_csrf_tags %>
@@ -90,6 +90,9 @@
 
     <%- unless customization_disabled? %>
       <%= theme_lookup("header") %>
+    <%- end %>
+
+    <%- if allow_plugins? %>
       <%= build_plugin_html 'server:header' %>
     <%- end %>
 
@@ -115,6 +118,9 @@
     <%- unless customization_disabled? %>
       <%= theme_lookup("body_tag") %>
     <%- end %>
+
+    <%- if allow_plugins? %>
       <%= build_plugin_html 'server:before-body-close' %>
+    <%- end %>
   </body>
 </html>

app/views/layouts/embed.html.erb

@@ -3,7 +3,7 @@
   <head>
     <meta charset="utf-8">
     <meta name="viewport" content="width=device-width, initial-scale=1.0, minimum-scale=1.0, user-scalable=yes, viewport-fit=cover">
-    <%= discourse_stylesheet_link_tag 'embed', theme_ids: nil %>
+    <%= discourse_stylesheet_link_tag 'embed', theme_id: nil %>
     <%- unless customization_disabled? %>
       <%= discourse_stylesheet_link_tag  :embedded_theme %>
     <%- end %>

app/views/layouts/finish_installation.html.erb

@@ -1,6 +1,6 @@
 <html>
   <head>
-    <%= discourse_stylesheet_link_tag 'wizard', theme_ids: nil %>
+    <%= discourse_stylesheet_link_tag 'wizard', theme_id: nil %>
     <%= discourse_color_scheme_stylesheets %>
 
     <%= render partial: "layouts/head" %>

app/views/layouts/no_ember.html.erb

@@ -8,13 +8,22 @@
   <%= render partial: "common/discourse_stylesheet" %>
   <%= discourse_csrf_tags %>
 
+  <%- unless customization_disabled? %>
     <%= theme_lookup("head_tag") %>
+  <%- end %>
+
   <%= yield(:no_ember_head) %>
+
+  <%- if allow_plugins? %>
     <%= build_plugin_html 'server:before-head-close' %>
+  <%- end -%>
 </head>
 <body class="no-ember <%= @custom_body_class %>">
   <%- unless customization_disabled? %>
     <%= theme_lookup("header") %>
+  <%- end %>
+
+  <%- if allow_plugins? %>
     <%= build_plugin_html 'server:header' %>
   <%- end %>
 
@@ -24,9 +33,15 @@
       <%= yield %>
     </div>
   </section>
+
+  <%- unless customization_disabled? %>
     <%= theme_lookup("footer") %>
     <%= theme_lookup("body_tag") %>
+  <%- end %>
+
+  <%- if allow_plugins? %>
     <%= build_plugin_html 'no-client:footer' %>
     <%= build_plugin_html 'server:before-body-close' %>
+  <%- end %>
 </body>
 </html>

app/views/qunit/index.html.erb

@@ -3,8 +3,8 @@
   <head>
     <title>QUnit Test Runner</title>
     <%= discourse_color_scheme_stylesheets %>
-    <%= discourse_stylesheet_link_tag(:desktop, theme_ids: nil) %>
+    <%= discourse_stylesheet_link_tag(:desktop, theme_id: nil) %>
-    <%= discourse_stylesheet_link_tag(:test_helper, theme_ids: nil) %>
+    <%= discourse_stylesheet_link_tag(:test_helper, theme_id: nil) %>
     <%= preload_script "discourse/tests/test_helper" %>
     <%= preload_script "discourse/tests/core_plugins_tests" %>
     <%= preload_script "discourse/tests/test_starter" %>

app/views/qunit/theme.html.erb

@@ -4,8 +4,8 @@
     <title>Theme QUnit Test Runner</title>
     <%= discourse_color_scheme_stylesheets %>
     <%- if !@suggested_themes %>
-      <%= discourse_stylesheet_link_tag(:desktop, theme_ids: nil) %>
+      <%= discourse_stylesheet_link_tag(:desktop, theme_id: nil) %>
-      <%= discourse_stylesheet_link_tag(:test_helper, theme_ids: nil) %>
+      <%= discourse_stylesheet_link_tag(:test_helper, theme_id: nil) %>
       <%= preload_script "locales/en" %>
       <%= preload_script "discourse/tests/theme_qunit_ember_jquery" %>
       <%= preload_script "discourse/tests/theme_qunit_vendor" %>

app/views/wizard/index.html.erb

@@ -1,6 +1,6 @@
 <html class="discourse-wizard">
   <head>
-    <%= discourse_stylesheet_link_tag :wizard, theme_ids: nil %>
+    <%= discourse_stylesheet_link_tag :wizard, theme_id: nil %>
     <%= discourse_color_scheme_stylesheets %>
     <%= preload_script "locales/#{I18n.locale}" %>
     <%- if ExtraLocalesController.client_overrides_exist? %>

app/views/wizard/qunit.html.erb

@@ -2,8 +2,8 @@
 <html>
   <head>
     <title>QUnit Test Runner</title>
-    <%= discourse_stylesheet_link_tag(:test_helper, theme_ids: nil) %>
+    <%= discourse_stylesheet_link_tag(:test_helper, theme_id: nil) %>
-    <%= discourse_stylesheet_link_tag :wizard, theme_ids: nil %>
+    <%= discourse_stylesheet_link_tag :wizard, theme_id: nil %>
     <%= preload_script "wizard/test/test_helper" %>
     <%= csrf_meta_tags %>
     <script src="<%= ExtraLocalesController.url("wizard") %>"></script>

lib/stylesheet/importer.rb

@@ -173,6 +173,8 @@ module Stylesheet
     end
 
     def theme_import(target)
+      return "" if !@theme_id
+
       attr = target == :embedded_theme ? :embedded_scss : :scss
       target = target.to_s.gsub("_theme", "").to_sym
 

lib/stylesheet/manager.rb

@@ -134,7 +134,7 @@ class Stylesheet::Manager
   attr_reader :theme_ids
 
   def initialize(theme_id: nil)
-    @theme_id = theme_id || SiteSetting.default_theme_id
+    @theme_id = theme_id
     @theme_ids = Theme.transform_ids(@theme_id)
     @themes_cache = {}
   end
@@ -218,16 +218,23 @@ class Stylesheet::Manager
 
       scss_checker = ScssChecker.new(target, stale_theme_ids)
 
-      load_themes(stale_theme_ids).each do |theme|
+      themes = @theme_id.blank? ? [nil] : load_themes(stale_theme_ids)
-        theme_id = theme.id
+
+      themes.each do |theme|
+        theme_id = theme&.id
         data = { target: target, theme_id: theme_id }
         builder = Builder.new(target: target, theme: theme, manager: self)
+        is_theme = builder.is_theme?
+        has_theme = builder.theme.present?
 
-        next if builder.theme.component && !scss_checker.has_scss(theme_id)
+        if is_theme && !has_theme
+          next
+        else
+          next if builder.theme&.component && !scss_checker.has_scss(theme_id)
           builder.compile unless File.exists?(builder.stylesheet_fullpath)
           href = builder.stylesheet_path(current_hostname)
-
           cache.defer_set("path_#{target}_#{theme_id}_#{current_hostname}", href)
+        end
 
         data[:new_href] = href
         stylesheets << data
@@ -239,7 +246,7 @@ class Stylesheet::Manager
   end
 
   def color_scheme_stylesheet_details(color_scheme_id = nil, media)
-    theme_id = @theme_ids.first
+    theme_id = @theme_id || SiteSetting.default_theme_id
 
     color_scheme = begin
       ColorScheme.find(color_scheme_id)

lib/stylesheet/manager/builder.rb

@@ -3,7 +3,7 @@
 class Stylesheet::Manager::Builder
   attr_reader :theme
 
-  def initialize(target: :desktop, theme:, color_scheme: nil, manager:)
+  def initialize(target: :desktop, theme: nil, color_scheme: nil, manager:)
     @target = target
     @theme = theme
     @color_scheme = color_scheme
@@ -115,11 +115,11 @@ class Stylesheet::Manager::Builder
 
   def qualified_target
     if is_theme?
-      "#{@target}_#{theme.id}"
+      "#{@target}_#{theme&.id}"
     elsif @color_scheme
       "#{@target}_#{scheme_slug}_#{@color_scheme&.id.to_s}"
     else
-      scheme_string = theme && theme.color_scheme ? "_#{theme.color_scheme.id}" : ""
+      scheme_string = theme&.color_scheme ? "_#{theme.color_scheme.id}" : ""
       "#{@target}#{scheme_string}"
     end
   end
@@ -186,10 +186,10 @@ class Stylesheet::Manager::Builder
   end
 
   def settings_digest
-    theme_ids = Theme.is_parent_theme?(theme.id) ? @manager.theme_ids : [theme.id]
-
     themes =
-      if Theme.is_parent_theme?(theme.id)
+      if !theme
+        []
+      elsif Theme.is_parent_theme?(theme.id)
         @manager.load_themes(@manager.theme_ids)
       else
         [@manager.get_theme(theme.id)]
@@ -211,7 +211,7 @@ class Stylesheet::Manager::Builder
   def uploads_digest
     sha1s = []
 
-    theme.upload_fields.map do |upload_field|
+    (theme&.upload_fields || []).map do |upload_field|
       sha1s << upload_field.upload.sha1
     end
 
@@ -247,7 +247,9 @@ class Stylesheet::Manager::Builder
 
   def resolve_baked_field(target, name)
     theme_ids =
-      if Theme.is_parent_theme?(theme.id)
+      if !theme
+        []
+      elsif Theme.is_parent_theme?(theme.id)
         @manager.theme_ids
       else
         [theme.id]

lib/svg_sprite/svg_sprite.rb

@@ -358,7 +358,7 @@ 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 |fname|
       next if !File.exist?(fname)
 
       svg_file = Nokogiri::XML(File.open(fname))
@@ -381,7 +381,7 @@ 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 |fname|
       next if !File.exist?(fname)
 
       svg_file = Nokogiri::XML(File.open(fname))
@@ -465,9 +465,10 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
     return [] if theme_id.blank?
 
     theme_icon_settings = []
+    theme_ids = Theme.transform_ids(theme_id)
 
     # Need to load full records for default values
-    Theme.where(id: Theme.transform_ids(theme_id)).each do |theme|
+    Theme.where(id: 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('|')
@@ -475,7 +476,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
       end
     end
 
-    theme_icon_settings |= ThemeModifierHelper.new(theme_ids: [theme_id]).svg_icons
+    theme_icon_settings |= ThemeModifierHelper.new(theme_ids: theme_ids).svg_icons
 
     theme_icon_settings
   end

lib/theme_modifier_helper.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 class ThemeModifierHelper
   def initialize(request: nil, theme_ids: nil)
-    @theme_ids = theme_ids || [request&.env&.[](:resolved_theme_id)]
+    @theme_ids = theme_ids || Theme.transform_ids(request&.env&.[](:resolved_theme_id))
   end
 
   ThemeModifierSet.modifiers.keys.each do |modifier|

spec/components/stylesheet/manager_spec.rb

@@ -12,12 +12,6 @@ describe Stylesheet::Manager do
     Theme.clear_default!
     link = manager.stylesheet_link_tag(:embedded_theme)
     expect(link).to eq("")
-
-    theme = Fabricate(:theme)
-    SiteSetting.default_theme_id = theme.id
-
-    link = manager.stylesheet_link_tag(:embedded_theme)
-    expect(link).not_to eq("")
   end
 
   it "still returns something for no themes" do

spec/components/svg_sprite/svg_sprite_spec.rb

@@ -139,12 +139,21 @@ describe SvgSprite do
 
   it 'includes icons defined in theme modifiers' do
     theme = Fabricate(:theme)
+    child_theme = Fabricate(:theme, component: true)
+    theme.add_relative_theme!(:child, child_theme)
 
     expect(SvgSprite.all_icons(theme.id)).not_to include("dragon")
 
     theme.theme_modifier_set.svg_icons = ["dragon"]
     theme.save!
-    expect(SvgSprite.all_icons(theme.id)).to include("dragon")
+
+    child_theme.theme_modifier_set.svg_icons = ["fly"]
+    child_theme.save!
+
+    icons = SvgSprite.all_icons(theme.id)
+
+    expect(icons).to include("dragon")
+    expect(icons).to include("fly")
   end
 
   it 'includes custom icons from a sprite in a theme' do

spec/fixtures/onebox/discourse_topic.response

@@ -9,7 +9,7 @@
 [image]
 
 And that too in just over an year, way to go! [boom]">
-    <meta name="discourse_theme_ids" content="28">
+    <meta name="discourse_theme_id" content="28">
     <meta name="discourse_current_homepage" content="latest">
     <meta name="generator" content="Discourse 2.4.0.beta10 - https://github.com/discourse/discourse version c84652eb8b2bb9dd616056621ee7c7b91fcf64a2">
 <link rel="icon" type="image/png" href="https://d11a6trkgmumsb.cloudfront.net/optimized/3X/b/3/b33be9538df3547fcf9d1a51a4637d77392ac6f9_2_32x32.png">

spec/fixtures/onebox/discourse_topic_reply.response

@@ -9,7 +9,7 @@
 [image]
 
 And that too in just over an year, way to go! [boom]">
-    <meta name="discourse_theme_ids" content="28">
+    <meta name="discourse_theme_id" content="28">
     <meta name="discourse_current_homepage" content="latest">
     <meta name="generator" content="Discourse 2.4.0.beta10 - https://github.com/discourse/discourse version c84652eb8b2bb9dd616056621ee7c7b91fcf64a2">
 <link rel="icon" type="image/png" href="https://d11a6trkgmumsb.cloudfront.net/optimized/3X/b/3/b33be9538df3547fcf9d1a51a4637d77392ac6f9_2_32x32.png">

spec/lib/content_security_policy_spec.rb

@@ -274,7 +274,7 @@ describe ContentSecurityPolicy do
     }
 
     def theme_policy
-      policy([theme.id])
+      policy(theme.id)
     end
 
     it 'can be extended by themes' do
@@ -303,13 +303,23 @@ describe ContentSecurityPolicy do
       theme.theme_modifier_set.csp_extensions = ["script-src: https://from-theme-flag.script", "worker-src: from-theme-flag.worker"]
       theme.save!
 
+      child_theme = Fabricate(:theme, component: true)
+      theme.add_relative_theme!(:child, child_theme)
+      child_theme.theme_modifier_set.csp_extensions = ["script-src: https://child-theme-flag.script", "worker-src: child-theme-flag.worker"]
+      child_theme.save!
+
       expect(parse(theme_policy)['script-src']).to include('https://from-theme-flag.script')
+      expect(parse(theme_policy)['script-src']).to include('https://child-theme-flag.script')
       expect(parse(theme_policy)['worker-src']).to include('from-theme-flag.worker')
+      expect(parse(theme_policy)['worker-src']).to include('child-theme-flag.worker')
 
       theme.destroy!
+      child_theme.destroy!
 
       expect(parse(theme_policy)['script-src']).to_not include('https://from-theme-flag.script')
       expect(parse(theme_policy)['worker-src']).to_not include('from-theme-flag.worker')
+      expect(parse(theme_policy)['worker-src']).to_not include('from-theme-flag.worker')
+      expect(parse(theme_policy)['worker-src']).to_not include('child-theme-flag.worker')
     end
 
     it 'is extended automatically when themes reference external scripts' do
@@ -352,7 +362,7 @@ describe ContentSecurityPolicy do
     end.to_h
   end
 
-  def policy(theme_ids = [], path_info: "/")
+  def policy(theme_id = nil, path_info: "/")
-    ContentSecurityPolicy.policy(theme_ids, path_info: path_info)
+    ContentSecurityPolicy.policy(theme_id, path_info: path_info)
   end
 end

spec/requests/bootstrap_controller_spec.rb

@@ -27,7 +27,7 @@ describe BootstrapController do
     bootstrap = json['bootstrap']
     expect(bootstrap).to be_present
     expect(bootstrap['title']).to be_present
-    expect(bootstrap['theme_ids']).to eq([theme.id])
+    expect(bootstrap['theme_id']).to eq(theme.id)
     expect(bootstrap['setup_data']['base_url']).to eq(Discourse.base_url)
     expect(bootstrap['stylesheets']).to be_present
 

spec/requests/safe_mode_controller_spec.rb

@@ -10,10 +10,18 @@ RSpec.describe SafeModeController do
       theme.save!
       theme.set_default!
 
+      Fabricate(:admin) # Avoid wizard page
+
+      get '/'
+
+      expect(response.status).to eq(200)
+      expect(response.body).to include("data-theme-id=\"#{theme.id}\"")
+
       get '/safe-mode'
 
       expect(response.status).to eq(200)
       expect(response.body).not_to include("My Custom Header")
+      expect(response.body).not_to include("data-theme-id=\"#{theme.id}\"")
     end
   end
 

spec/requests/svg_sprite_controller_spec.rb

@@ -16,7 +16,7 @@ describe SvgSpriteController do
       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"
+      get "/svg-sprite/#{Discourse.current_hostname}/svg-#{theme.id}-#{SvgSprite.version(theme.id)}.js"
       expect(response.status).to eq(200)
     end
 

spec/serializers/listable_topic_serializer_spec.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+describe ListableTopicSerializer do
+  fab!(:topic) { Fabricate(:topic) }
+
+  describe '#excerpt' do
+    it 'can be extended by theme modifiers' do
+      payload = TopicListItemSerializer.new(topic,
+        scope: Guardian.new,
+        root: false
+      ).as_json
+
+      expect(payload[:excerpt]).to eq(nil)
+
+      theme = Fabricate(:theme)
+
+      child_theme = Fabricate(:theme, component: true).tap do |t|
+        theme.add_relative_theme!(:child, t)
+      end
+
+      child_theme.theme_modifier_set.serialize_topic_excerpts = true
+      child_theme.save!
+
+      request = ActionController::TestRequest.new(
+        { resolved_theme_id: theme.id },
+        nil,
+        nil
+      )
+
+      guardian = Guardian.new(nil, request)
+
+      payload = TopicListItemSerializer.new(topic,
+        scope: guardian,
+        root: false
+      ).as_json
+
+      expect(payload[:excerpt]).to eq(topic.excerpt)
+    end
+  end
+end