From 1c1d68728305f86d7f963a895d50968e83bd8a97 Mon Sep 17 00:00:00 2001 From: Kelv Date: Tue, 1 Apr 2025 10:54:48 +0800 Subject: [PATCH] DEV: show no icon if icon name is missing and log at error level (#31866) On 1st April 2025, we start showing no icons if the icon name used is a deprecated one and therefore no longer part of the svg set. We'll continue showing the messages with the correct icon name to aid correction of these names. Console logging will now be done at an error level for such icons. We retain the behaviour of raising an error for such icons in plugins from svg_sprite.rb in test environments, but removed this from icon-library.js as it's harder to test the actual expected behaviour of returning the original icon now that it's not part of the deprecation workflow. (sinon.stub doesn't work well here for `isTesting` - the alternative would be to override the environment.js module with `proxyquire`) In any case, once we remove the mapping logic, we won't be raising errors in test environment either for this scenario. --- .../discourse/app/lib/icon-library.js | 20 +++----- .../services/deprecation-warning-handler.js | 1 - .../tests/unit/lib/icon-library-test.js | 50 ++++--------------- lib/deprecated_icon_handler.rb | 11 ++-- lib/discourse.rb | 4 ++ spec/lib/svg_sprite/svg_sprite_spec.rb | 14 +++--- 6 files changed, 33 insertions(+), 67 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/icon-library.js b/app/assets/javascripts/discourse/app/lib/icon-library.js index d107a52e0f8..88559fbc952 100644 --- a/app/assets/javascripts/discourse/app/lib/icon-library.js +++ b/app/assets/javascripts/discourse/app/lib/icon-library.js @@ -1,11 +1,7 @@ import { h } from "virtual-dom"; import attributeHook from "discourse/lib/attribute-hook"; import deprecated from "discourse/lib/deprecated"; -import { - isDevelopment, - isRailsTesting, - isTesting, -} from "discourse/lib/environment"; +import { isDevelopment } from "discourse/lib/environment"; import escape from "discourse/lib/escape"; import { i18n } from "discourse-i18n"; @@ -146,17 +142,13 @@ function handleDeprecatedIcon(id) { newId = remapFromFA5(newId); if (newId !== id) { - deprecated( - `The icon name "${id}" has been updated to "${newId}". Please use the new name in your code. Old names will be removed in Q2 2025.`, - { - id: "discourse.fontawesome-6-upgrade", - url: "https://meta.discourse.org/t/325349", - raiseError: isTesting() || isRailsTesting(), - } - ); + const error_msg = `Missing icon error: The icon name "${id}" has been removed and should be updated to "${newId}" in your code. More info at https://meta.discourse.org/t/325349.`; + + // eslint-disable-next-line no-console + console.error(error_msg); } - return newId; + return id; } function warnIfMissing(id) { diff --git a/app/assets/javascripts/discourse/app/services/deprecation-warning-handler.js b/app/assets/javascripts/discourse/app/services/deprecation-warning-handler.js index 276c510e417..935f7936a57 100644 --- a/app/assets/javascripts/discourse/app/services/deprecation-warning-handler.js +++ b/app/assets/javascripts/discourse/app/services/deprecation-warning-handler.js @@ -19,7 +19,6 @@ export const CRITICAL_DEPRECATIONS = [ "discourse.header-widget-overrides", "discourse.plugin-outlet-tag-name", "discourse.post-menu-widget-overrides", - "discourse.fontawesome-6-upgrade", "discourse.add-flag-property", "discourse.breadcrumbs.childCategories", "discourse.breadcrumbs.firstCategory", diff --git a/app/assets/javascripts/discourse/tests/unit/lib/icon-library-test.js b/app/assets/javascripts/discourse/tests/unit/lib/icon-library-test.js index 1c3df6d6dea..b1aff99954d 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/icon-library-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/icon-library-test.js @@ -1,15 +1,10 @@ import { setupTest } from "ember-qunit"; import { module, test } from "qunit"; -import { withSilencedDeprecations } from "discourse/lib/deprecated"; import { convertIconClass, iconHTML, iconNode, } from "discourse/lib/icon-library"; -import { - disableRaiseOnDeprecation, - enableRaiseOnDeprecation, -} from "discourse/tests/helpers/raise-on-deprecation"; module("Unit | Utility | icon-library", function (hooks) { setupTest(hooks); @@ -52,43 +47,18 @@ module("Unit | Utility | icon-library", function (hooks) { }); test("fa5 remaps", function (assert) { - withSilencedDeprecations("discourse.fontawesome-6-upgrade", () => { - const adjustIcon = iconHTML("adjust"); - assert.true(adjustIcon.includes("d-icon-adjust"), "class is maintained"); - assert.true( - adjustIcon.includes('href="#circle-half-stroke"'), - "has remapped icon" - ); + const adjustIcon = iconHTML("adjust"); + assert.true(adjustIcon.includes("d-icon-adjust"), "class is maintained"); + assert.true(adjustIcon.includes('href="#adjust"'), "keeps original icon"); - const farIcon = iconHTML("far-dot-circle"); - assert.true( - farIcon.includes("d-icon-far-dot-circle"), - "class is maintained" - ); - assert.true( - farIcon.includes('href="#far-circle-dot"'), - "has remapped icon" - ); - }); - }); - - test("fa5 remaps throws error", function (assert) { - disableRaiseOnDeprecation(); - assert.throws( - () => { - iconHTML("adjust"); - }, - /Deprecation notice: The icon name "adjust" has been updated to "circle-half-stroke".*\[deprecation id: discourse\.fontawesome-6-upgrade\]/, - "throws an error if icon name is deprecated" + const farIcon = iconHTML("far-dot-circle"); + assert.true( + farIcon.includes("d-icon-far-dot-circle"), + "class is maintained" ); - - assert.throws( - () => { - iconHTML("far-dot-circle"); - }, - /Deprecation notice: The icon name "far-dot-circle" has been updated to "far-circle-dot".*\[deprecation id: discourse\.fontawesome-6-upgrade\]/, - "throws an error if icon name is deprecated" + assert.true( + farIcon.includes('href="#far-dot-circle"'), + "keeps original icon" ); - enableRaiseOnDeprecation(); }); }); diff --git a/lib/deprecated_icon_handler.rb b/lib/deprecated_icon_handler.rb index f9b5cd2ba71..48eaac66704 100644 --- a/lib/deprecated_icon_handler.rb +++ b/lib/deprecated_icon_handler.rb @@ -687,11 +687,12 @@ module DeprecatedIconHandler new_name = remap_from_fa5(new_name) if icon_name != new_name - Discourse.deprecate( - "The icon `#{icon_name}` is deprecated. Use `#{new_name}` instead.", - raise_error: Rails.env.test?, - ) - return new_name + error_msg = + `Missing icon error: The icon name "#{icon_name}" has been removed and should be updated to "#{new_name}" in your code. More info at https://meta.discourse.org/t/325349.` + + Rails.logger.error(error_msg) + + raise Discourse::MissingIconError.new(error_msg) if Rails.env.test? end icon_name diff --git a/lib/discourse.rb b/lib/discourse.rb index 68c4dac2196..3041e3cb30c 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -117,6 +117,7 @@ module Discourse class CommandError < RuntimeError attr_reader :status, :stdout, :stderr + def initialize(message, status: nil, stdout: nil, stderr: nil) super(message) @status = status @@ -305,6 +306,9 @@ module Discourse class Deprecation < StandardError end + class MissingIconError < StandardError + end + class ScssError < StandardError end diff --git a/spec/lib/svg_sprite/svg_sprite_spec.rb b/spec/lib/svg_sprite/svg_sprite_spec.rb index 6f298b28d0d..cb23ae03f30 100644 --- a/spec/lib/svg_sprite/svg_sprite_spec.rb +++ b/spec/lib/svg_sprite/svg_sprite_spec.rb @@ -88,18 +88,18 @@ RSpec.describe SvgSprite do it "strips whitespace when processing icons" do Fabricate(:badge, name: "Custom Icon Badge", icon: " fab fa-facebook-messenger ") - expect(SvgSprite.all_icons).to include("fab-facebook-messenger") - expect(SvgSprite.all_icons).not_to include(" fab-facebook-messenger ") + expect(SvgSprite.all_icons).to include("fab fa-facebook-messenger") + expect(SvgSprite.all_icons).not_to include(" fab fa-facebook-messenger ") end - it "includes Font Awesome 5 icons from badges" do + it "includes icons from badges" do Fabricate(:badge, name: "Custom Icon Badge", icon: "far fa-building") - expect(SvgSprite.all_icons).to include("far-building") + expect(SvgSprite.all_icons).to include("far fa-building") end it "raises an error in test for deprecated icons" do allow(Rails.env).to receive(:test?).and_return(true) - expect { SvgSprite.search("fa-gamepad") }.to raise_error(Discourse::Deprecation) + expect { SvgSprite.search("fa-gamepad") }.to raise_error(Discourse::MissingIconError) end it "includes icons defined in theme settings" do @@ -127,7 +127,7 @@ RSpec.describe SvgSprite do # FA5 syntax theme.update_setting(:custom_icon, "fab fa-bandcamp") theme.save! - expect(SvgSprite.all_icons(theme.id)).to include("fab-bandcamp") + expect(SvgSprite.all_icons(theme.id)).to include("fab fa-bandcamp") # Internal Discourse syntax + multiple icons theme.update_setting(:custom_icon, "fab-android|dragon") @@ -227,7 +227,7 @@ RSpec.describe SvgSprite do DiscoursePluginRegistry.register_svg_icon "fab fa-bandcamp" expect(SvgSprite.all_icons).to include("blender") - expect(SvgSprite.all_icons).to include("fab-bandcamp") + expect(SvgSprite.all_icons).to include("fab fa-bandcamp") end it "includes Font Awesome icon from groups" do