Problem/Motivation

In core

There are at least 141 icons in Drupal Core, living in many folders:

  • misc/icons/{group}/{icon_id}.svg
  • misc/icons/{icon_id}.gif
  • misc/{icon_id}.gif
  • misc/{icon_id}.png
  • modules/media/images/icons/{icon_id}.png
  • the ones from umami theme
  • the ones from claro theme
  • the ones from olivero theme
  • the ones from starterkit_theme theme
  • ...

Used by the modules in many ways...

... from CSS backgrounds:

modules/options/css/options.icon.theme.pcss.css:  background-image: url(../../../misc/icons/55565b/selection_list.svg);
modules/ckeditor5/css/drupalmedia.css:  background: url("../../../misc/icons/e29700/warning.svg") no-repeat center 4px;
modules/settings_tray/css/settings_tray.theme.css:  background-image: url(../../../misc/icons/bebebe/pencil.svg);
...

... from PHP:

modules/filter/filter.module: $image->setAttribute('src', base_path() . 'core/misc/icons/e32700/error.svg');
modules/update/update.report.inc: $uri = 'core/misc/icons/73b355/check.svg';
modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestImageButtonForm.php: '#src' => 'core/misc/icons/787878/cog.svg',
...

... from Twig:

modules/navigation/templates/toolbar-button.html.twig: icon ? 'toolbar-button--icon--' ~ icon : '',
modules/update/templates/update-project-status.html.twig: {{ status.icon }}
...

However:

  • This usage is unorganized. If an icon is moved or changed, we need to manually track its usage
  • Everything is hardcoded. There is no UI to use or change those icons from admin pages
  • There is no way for themes to override icon packs or individual icons, in order to adapt them to specific branding

Having a well established API will make this possible.

In contrib

There are many modules providing icons management for Drupal 10 and/or D11. However, most of them are dedicated to a specific icon pack:

Or a specific remote service:

Hopefully, there are few agnostic icon management modules:

Anyway, there are a lot of duplicate work on those modules:

  • On the lower (API) level: most of them are providing Form Element, Render Element, Twig Function… which works more or less the same and can be unified
  • On the higher level: they provide more or less the same integration with Drupal API (fields, menus, media, ckeditor5, etc.)

A common Core API will allow those modules to focus on the specific value they are bringing to the Drupal community, instead of duplicating the same features over and over.

Modules with icon integrations:

Proposed resolution

The starting point

We need an unified API to declare icon packs, retrieve the icons list, and interacts with each icon (form, render, metadata…). And it is better to start from an existing implementation from contrib space.

Among the agnostic modules, we propose to get inspiration from ui_icons because:

  • iconset is not front-dev friendly enough (icon sets delcaration in YML and icon handling in PHP are separated, but the later do too much) and has a dependency to toolshed
  • icons is not front-dev friendly enough (icon library declaration and icon extraction process are merged in a PHP class)

Also, ui_icons have those advantages:

  • Tested with many icon packs: bootstrap, dsfr, feather, fontawesome, heroicons, icomoon, material, octicons, phosphor, remix, uswds...
  • Small & clean API: 22 PHP files, 1 177 lines of code, 1 022 lines of comments. Already 95% of tests coverage.
  • Built by the UI Suite team which is dedicated to design systems implementation in Drupal since early 2020 (early 2017 if we count the early efforts), so with a deep care for design & site building needs.

(Disclaimer: I am part of UI Suite and one of the maintainers of the ui_icons module)

The main module of ui_icons is providing those elements:

  • a definition class for icons
    icon_id
source: URL or path of the icon
data: additional data generated by and specific to the extractor
group
  • a plugin manager for icon packs with a YAML discovery, which is front-dev friendly (declarative, straightforward, without drupalism, using JSON schema) def
  • a plugin manager for extractors with a PHP attribute discovery, to build the icon list from the icon pack definition
    function discoverIcons(): array;
function label(): string;
function description(): string;
static function createIcon(string $icon_id, string $path, array $data, ?string $group = NULL): IconDefinition;
function getFilesFromSources(array $sources, array $paths): array;
  • a render element and the related Twig function:
    [
 '#type' => 'icon',
 '#icon_pack' => 'material_symbols',
 '#icon' => 'home',
 '#settings' => ['width' => 64],
]
  • a form element
    [
 '#type' => 'icon_autocomplete',
 '#title' => $this->t('Select icon'),
 '#default_value' => 'my_icon_pack:my_default_icon',
 '#allowed_icon_pack' => ['my_icon_pack, 'other_icon_pack'],
]
  • a few extractor plugins: Manual declaration (to be confirmed), Local path, SVG, SVG Sprite, and Font/Codepoint (to be implemented)

Proposal 1: low-level, complete API

In this proposal, we simply take everything the main module is providing.

Then, every contrib module (including UI Icons which is still relevant) can provide higher level features:

  • Icon library, to browse icons from the user interface
  • Alternative icon pickers form elements
  • Integration with various Drupal Core & Contrib API
  • New extractors, including niche formats and integration with remote services
  • ...

Proposal 2: still low-level, but compact API

We take only what would be needed by core today, so without the form element and most of the extractors:

  • the definition class for icons
  • the plugin manager for icon packs with a YAML discovery
  • the plugin manager for extractors with a PHP attribute discovery
  • the render element and the related Twig function
  • the extractor(s) needed for Core icons (Local path?)

Remaining/others tasks

First, let's decide what are we proposing for integration.

About naming, is it possible to claim /project/icon namespace? This (popular, 6,500 installs) module is deprecated, unsupported, unmaintained since 2017 and Drupal 7 era.

Once implemented, it would be great for Drupal Core to define icon pack(s) for its own icons and start to use them in PHP and Twig.

User interface changes

No, because the module will not provide integration with Drupal admin pages (ckeditor5, field management, menu management...), at least at the beginning, but contrib modules like UI Icons will do that. We hope to see an ecosystem emerging, and existing modules migrating to the new API.

Introduced terminology

"Icon", "Icon pack", "Extractors"... what else?

API changes

No, we add a new API but we don't change existing.

Data model changes

No.

Release notes snippet

To be done.

CommentFileSizeAuthor
#32 3471494-nr-bot.txt90 bytesneeds-review-queue-bot
#30 3471494-nr-bot.txt90 bytesneeds-review-queue-bot
iconpack_definition.png146.5 KBpdureau

Issue fork drupal-3471494

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Issue summary: View changes
quietone’s picture

quietone commented
Version: 11.0.x-dev » 11.x-dev

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue summary: View changes

Some typos

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

As co-maintainer of the new core Navigation and as a core FEFM and UX co-maintainer I'm really interested on seeing this happening. Great work so far.

catch’s picture

catch
he/him
Primary language English
commented

About naming, is it possible to claim https://www.drupal.org/project/icon namespace?

For the core module I don't see why not and I'm not sure we need to do anything special to 'take' it for core given there's no Drupal 8+ code there, but we could file a project ownership issue just in case maybe?

Proposal 2 sounds like it would be easier to get into core due to the lower surface area/amount of code to review - if we did that, would the existing contrib module continue to provide those bits? We can always add more to core later.

pdureau’s picture

pdureau commented

I'm not sure we need to do anything special to 'take' it for core given there's no Drupal 8+ code ther

👍

We could file a project ownership issue just in case maybe?

It would be safer. However, will it make us responsible for the maintenance of the existing codebase? What will happen to https://www.drupal.org/project/issues/icon ?

if we did that, would the existing contrib module continue to provide those bits?

That's the idea, https://www.drupal.org/project/ui_icons will still exist and leverage the API found in Core by providing:

  • other extractor plugins
  • integration with Drupal API & tools
  • some extra stuff like library pages, form improvements...

We hope many other contrib modules will do the same.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It could take weeks looking at #3376179: Offer to maintain Icon API

catch’s picture

catch
he/him
Primary language English
commented

It would be safer. However, will it make us responsible for the maintenance of the existing codebase? What will happen to https://www.drupal.org/project/issues/icon

Thinking about it - we might just need the reverse version of #3421791: Ensure that Book does not get special core treatment. Once Drupal 7 is out of support (January 2025), we could then look at locking down the project so no Drupal 8+ code can be published there either, not sure how all of that is going to work.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

We discussed this proposal with the UI Suite team a few times, one of them with the Experience Builder team including Lauri and Wim. They also expressed their interest on seeign this going into core as a helpful step for Experience Builder.
Also, Lauri gave his +1 as a Product Manager to this feature (posting this with his permission).

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau
Status: Active » Needs work

I assign the issue to me, to work with mogtofu33

It would not be a module but moved to Drupal\Core (so to Drupal\Core\Theme I guess)

We will add a MR to 11.x branch with the stuff mandatory for usage in Core (navigation module for now) :

  • iconpack plugin type with its plugin manager
  • extractor plugin type with its plugin manager & its PHP attribute notation
  • (revamped) icon finder service
  • icon() Twig function & ui_icon render element (renamed icon)
  • only svg & svg_sprite extractors (the others extractors will stay in the ui_icons contrib module)
  • the related tests

Also, the 2 form elements (same MR or distinct? Let's decide):

  • icon_autocomplete from main module
  • icon_picker from ui_icon_picker module

Once done, we will add a core issue and a MR for navigation module with:

  • an iconpack declaration to get the naivgation icons
  • loading those icons fthrough the new API instead that the current way
finnsky’s picture

finnsky commented

Great News! It will unblock tonns of issues here!!!

pdureau’s picture

pdureau commented

A few notes.

Decisions

We will wait ui_icons 1.0.0-beta2 before proposing a MR.

Also, the 2 form elements (same MR or distinct? Let's decide):

  • icon_autocomplete from main module
  • icon_picker from ui_icon_picker module

The 2 form elements will be discussed in their own issue (and their own MR).

Jean (mogtofu33):

For path not included, is it because it seems every icons are svg?
SVG extractor include the content of the icon, so it's heavier than the simple path with &lt:img src="">, I think path is better and lighter for clean svgs pack (not mixed size/viewport).

We will also propose path extractor in the MR.

Namespace mapping

Let's define the destination path in Drupal Core namespace.

Icon definition, icon pack management, and their exceptions:

Drupal\ui_icons\Exception\IconDefinitionInvalidDataException >> Drupal\Core\Theme\???
Drupal\ui_icons\Exception\IconPackConfigErrorException >> Drupal\Core\Theme\???
Drupal\ui_icons\IconDefinitionInterface >> Drupal\Core\Theme\???
Drupal\ui_icons\IconDefinition >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconPackManagerInterface >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconPackManager >> Drupal\Core\Theme\???

Rendering:

Drupal\ui_icons\Element\Icon >> Drupal\Core\Theme\???
Drupal\ui_icons\TemplateIconTwigExtension >> Drupal\Core\Theme\???

Icon extractors API and the icon finder:

Drupal\ui_icons\Attribute\IconExtractor >> Drupal\Core\Theme\???
Drupal\ui_icons\Form\IconExtractorSettingsForm >> Drupal\Core\Theme\???
Drupal\ui_icons\IconFinderInterface >> Drupal\Core\Theme\???
Drupal\ui_icons\IconFinder >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractorBase >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractorInterface >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractorPluginManager >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractorWithFinderInterface >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractorWithFinder >> Drupal\Core\Theme\???
Drupal\ui_icons\PluginForm\IconPackExtractorForm >> Drupal\Core\Theme\???

Some extractor plugins:

Drupal\ui_icons\Plugin\IconExtractor\PathExtractor >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractor\SvgExtractor >> Drupal\Core\Theme\???
Drupal\ui_icons\Plugin\IconExtractor\SvgSpriteExtractor >> Drupal\Core\Theme\???
mogtofu33’s picture

mogtofu33 commented
Assigned: pdureau » mogtofu33
catch’s picture

catch
he/him
Primary language English
commented

edit: completely misunderstood the comment which was saying similar things to what I was saying already. Deleted to avoid confusion.

baluertl’s picture

baluertl
he/him
Primary language Hungarian
Location Budapest 🇪🇺
commented
Issue summary: View changes
gxleano’s picture

gxleano commented

The Iconify Icons module already includes integration with UI Icons, allowing it to be used as a provider by installing the Iconify Icons Provider submodule.

pdureau’s picture

pdureau commented

Pierre:

About naming, is it possible to claim /project/icon namespace? This (popular, 6,500 installs) module is deprecated, unsupported, unmaintained since 2017 and Drupal 7 era.

Andy:

It could take weeks looking at #3376179: Offer to maintain Icon API

Catch:

For the core module I don't see why not and I'm not sure we need to do anything special to 'take' it for core given there's no Drupal 8+ code there, but we could file a project ownership issue just in case maybe?

Proposal 2 sounds like it would be easier to get into core due to the lower surface area/amount of code to review - if we did that, would the existing contrib module continue to provide those bits? We can always add more to core later.

In order to not experience again the hassle of #3352256: Move code from the experimental SDC module to core, we decided during the Barcelona meeting to make this API directly a Drupal\Core sub-system instead of an experimental module.

catch’s picture

catch
he/him
Primary language English
commented

Yes if we put this in Drupal\Core we don't need to worry about the Drupal\icon namespace at all.

pdureau’s picture

pdureau commented

The ui_icons beta2 is coming soon, so the MR is coming soon.

What will be the destination paths in Drupal Core namespace?

Icon definition, icon pack management, and their exceptions, inside \Theme\Icon:

Drupal\ui_icons\Exception\IconDefinitionInvalidDataException >> Drupal\Core\Theme\Icon\Exception\IconDefinitionInvalidDataException
Drupal\ui_icons\Exception\IconPackConfigErrorException >> Drupal\Core\Theme\Icon\Exception\IconPackConfigErrorException
Drupal\ui_icons\IconDefinitionInterface >> Drupal\Core\Theme\Icon\IconDefinitionInterface
Drupal\ui_icons\IconDefinition >> Drupal\Core\Theme\Icon\IconDefinition
Drupal\ui_icons\Plugin\IconPackManagerInterface >> Drupal\Core\Theme\Icon\Plugin\IconPackManagerInterface
Drupal\ui_icons\Plugin\IconPackManager >> Drupal\Core\Theme\Icon\Plugin\IconPackManager

Rendering, outside of \Theme\, in the specific folders:

Drupal\ui_icons\Element\Icon >> Drupal\Core\Render\Element\Icon
Drupal\ui_icons\Template\IconTwigExtension >> Drupal\Core\Template\IconsTwigExtension (warning: from singular to plural, to comply with SDC)

Icon extractors API and the icon finder

Some inside \Theme to follow other Drupal\Core\ hierarchies:

Drupal\ui_icons\Attribute\IconExtractor >> Drupal\Core\Theme\Icon\Attribute\IconExtractor 
Drupal\ui_icons\Form\IconExtractorSettingsForm >> Drupal\Core\Theme\Icon\IconExtractorSettingsForm

the others inside \Theme\Icon:

Drupal\ui_icons\IconFinderInterface >> Drupal\Core\Theme\Icon\IconFinderInterface
Drupal\ui_icons\IconFinder >> Drupal\Core\Theme\Icon\IconFinder
Drupal\ui_icons\Plugin\IconExtractorBase >> Drupal\Core\Theme\Icon\IconExtractorBase
Drupal\ui_icons\Plugin\IconExtractorInterface >> Drupal\Core\Theme\Icon\IconExtractorInterface
Drupal\ui_icons\Plugin\IconExtractorPluginManager >> Drupal\Core\Theme\Icon\IconExtractorPluginManager
Drupal\ui_icons\Plugin\IconExtractorWithFinderInterface >> Drupal\Core\Theme\Icon\IconExtractorWithFinderInterface
Drupal\ui_icons\Plugin\IconExtractorWithFinder >> Drupal\Core\Theme\Icon\IconExtractorWithFinder
Drupal\ui_icons\PluginForm\IconPackExtractorForm >> Drupal\Core\Theme\Icon\IconPackExtractorForm

Some extractor plugins: in \Theme\Plugin\ to follow other Drupal\Core\ hierarchies:

Drupal\ui_icons\Plugin\IconExtractor\PathExtractor >> Drupal\Core\Theme\Plugin\IconExtractor\PathExtractor
Drupal\ui_icons\Plugin\IconExtractor\SvgExtractor >> Drupal\Core\Theme\Plugin\IconExtractor\SvgExtractor 
Drupal\ui_icons\Plugin\IconExtractor\SvgSpriteExtractor >> Drupal\Core\Theme\Plugin\IconExtractor\SvgSpriteExtractor

what do you think about that? it is just an humble proposal...

pdureau’s picture

pdureau commented

Some thoughts to share before the MR

With this Icons API, we target an harmonisation of the Drupal ecosystem of modules related to icons:

  • a single plugin manager and declaration format
  • a single extraction mechanism
  • and a single render element + Twig function.

But (most of) the actual modules stay relevant.

For example, material_icons:

  • is mainly providing 8 icon packs: it will still provide them but in a "normalized" way, so other modules can also leverage them
  • is also providing an integration with Field API & Ckeditor5: they can get rid of it and leverage an other module, or they can keep it and this feature will be compatible with other iconpacks provided by others themes or modules

Other example, iconset :

  • is not providing any iconpack
  • is providing its own icon API (YAML declaration, handlers, render element, twig function...): this will be overlapping with the new Icon API and can be removed
  • is mainly providing an integration with Drupal API (ckeditor5, field API, menu...): this must be kept and will now be usable will all iconpacks using the standardized icon API

A more complex example, iconify_icons :

  • is mainly providing an integration with https://iconify.design/ : this must be kept but reimplemented as an extractor plugin.
  • is also providing some low level tools (render element, twig function...): this will be overlapping with the new Icon API and can be removed
  • is also providing some integration with Drupal API (ckeditor5, field API...): this can be kept and will now be usable will all iconpacks using the standardized icon API
  • the 150 iconpacks can be provided without manually writing all YAML file thanks to hook_icon_pack_alter. For performance reason, they can be activate one by one in a UI provided by the module.
pdureau’s picture

pdureau commented

In the issue summary, there were initially 2 proposals:

  • Proposal 1: low-level, complete API. We simply take everything the main module is providing.
  • Proposal 2: still low-level, but compact API. We take only what would be needed by core today, so without the form element and most of the extractors.

The MR will be the proposal 2.

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes

mogtofu33 opened merge request !9870

nod_’s picture

nod_
Primary language French
Location Lille
commented

🎉

@pdureau, can you add credit to everyone that contributed to this so far on your side? Thanks!

pdureau’s picture

pdureau commented
Assigned: mogtofu33 » larowlan
Status: Needs work » Needs review

MR ready for review.

@pdureau, can you add credit to everyone that contributed to this so far on your side? Thanks!

Issue credit given to people which worked on the original UI Icons codebase: https://git.drupalcode.org/project/ui_icons/-/commits/1.0.x

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3471494-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3471494-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

matthieuscarset’s picture

matthieuscarset commented

I just discovered this issue so I'm late in the game apparently... but I have a question.

Why can't icons be content entities?

Implementation could be much simpler IMHO.

  • The new extractor is not necessary - we already have a Migrate API in place.
  • The new Twig function is not necessary neither - we coudl print a field from an icon content.
  • The concept of Pack is not necessary - we have taxonomies
  • An icon could be "translated" (big +1 here)
  • View integration, field formatters...etc works out of the box
  • JSON API ready
  • ... and certainly other things I haven't understood yet from the current MR ?
andypost’s picture

andypost
he/him
Primary language Russian
commented

Re #33 great idea, moreover it should allow apply access permissions to renderables

nod_’s picture

nod_
Primary language French
Location Lille
commented

Why can't icons be content entities?

Lifecycle of the data is different, a different object makes sense.

I'd hate to deal with icon pack update when everything is content, how do you start to write the update function for that in a way that works everywhere? Importing in content means duplicating the code for the icon, otherwise what's the point of having content you can't edit. And that opens the door to update problems if you've changed an icon and the source of your icons publish an update changing the same element how do you deal with that in a developer/user friendly way?

All that, or we can just realize that icons are not content and avoid most if not all those problems.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
Issue tags: +no-needs-review-bot
matthieuscarset’s picture

matthieuscarset commented

how do you start to write the update function for that in a way that works everywhere ?

Not sure but we have Recipes and Default content and Actions in core now.

how do you deal with that in a developer/user friendly way?

It is a challenge indeed but certainly easier to solver compare to the current suggested implementation. One solution out of my head: entity_reference_revisions.

I don't know if icons are not content but they are certainly a type of medias.

catch’s picture

catch
he/him
Primary language English
commented

If icons are content, they will be impossible to version control and deploy - you'd have to import icon packs on production.

matthieuscarset’s picture

matthieuscarset commented

Right, there would be no version control if icons are entities - although some might say we do have revisions on content entities, or it can be a hook_update or a recipe + action or default_content or a migrate process 🤷.

It is simpler to have icon files committed in core, I agree.

But I wanted to comment that we need to build a whole new set of functionality to handle all the future integrations (translation, permissions, categorizations, display...etc).

PS: as of now, don't we face the same issue ? We'll need to upload a new version of an icon pack if we want to update the files, right?

andypost’s picture

andypost
he/him
Primary language Russian
commented

Looking at Extraction API it can be easily wrapped into migration plugin so more icon-pack processors can be easily added

As I get caching is applied only for IconPackManager definitions so the rest of API will use file-search for every icon requested, hope I'm wrong...

I see no real usage of API (except unit tests), as there's render and forms fucntional test required, moreover it needs perf test to make sure the page is renderable when there's 100-200 icons

PS: reading IS not clear how new API is supposed to be consumed by core, as contrib already has solutions

pdureau’s picture

pdureau commented

Lifecycle of the data is different, a different object makes sense.

Indeed. If we split data in Drupal into 3 main layers ordered from the more deployable/sharable to the less, and from the colder (less frequent updates) to the hotter, we have: code, config & content.

We can build those rules to avoid any problems between environments & deploys:

  • content (ex: article node) can reference content (ex: node author), config (ex: article content type) & code (ex: node entity type)
  • config (ex: article content type) can reference config (ex: field storage) & code (ex: field type)
  • code (ex: field type) can only reference code (ex: another plugin or a service)

So, in order to be usable with direct, explicit references in content (ex: a field value or in Ckeditor5), config (ex: Entity View Display) and code (ex: Twig template) icon packs (and their icons) must be code.

Also, we (the MR authors) are promoting a design sytem methodology, where a full design system implementation is shippable & shareable in a single unit of code: the Drupal theme. You can think about this API as "the SDC of icons". So, it is definitely code.

matthieuscarset’s picture

matthieuscarset commented

I can see that we are building another SDC of icon.

It is clear the architectural decision has been made 👷‍♂️ so I don't really know what to say.

I wanted to point out some of the limitations that will certainly arise when real site builders will want to use icons.

nod_’s picture

nod_
Primary language French
Location Lille
commented

@matthieuscarset: I see your point when you use Drupal as an icon library creator, then everything is better as content. That's not the use case addressed by this work. This is optimized for integrating existing icon sets into Drupal. It's still possible to use drupal as an icon library ceator and integrate with this new API though :D

As a sidenote we don't have permission access or translation of JS and CSS files (it's not the files that are translated it's what's inside) it's the same for icons so not having everything that content entities for icons does not mean we will need to reimplement everything.

PS: reading IS not clear how new API is supposed to be consumed by core, as contrib already has solutions

Once this gets in, there will be work to make navigation use it, it'll be clearer then. There was extensive reseach on contrib solutions see #23 and I'm sure @pdureau can elaborate if needed.

mogtofu33’s picture

mogtofu33 commented

As I get caching is applied only for IconPackManager definitions so the rest of API will use file-search for every icon requested, hope I'm wrong...

@andypost, the result of the file search (when it's a file search based icon) is added to the `IconPackManager` definition, and then cached with it. Each icon request do not involve the file search again.

I see no real usage of API (except unit tests), as there's render and forms fucntional test required, moreover it needs perf test to make sure the page is renderable when there's 100-200 icons

On the perf side, when used with render api or twig function, it's just an inline template, no call to database.
As mentioned by @nod_ we will propose soon an implementation with core navigation module.

matthieuscarset’s picture

matthieuscarset commented

I read the related issues and I think I see where this one comes from. We want to a standard way of declaring/using icons across Drupal (e.g. for menu links for instance). We want to be able to declare it in YAML files. So we need a new plugin manager👌.

We are true YAML lovers.

pdureau’s picture

pdureau commented
Status: Needs review » Needs work

Once this gets in, there will be work to make navigation use it, it'll be clearer then. There was extensive reseach on contrib solutions see #23 and I'm sure @pdureau can elaborate if needed.

This second MR is planned for this week and will have its dedicated issue.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Assigned: larowlan » Unassigned
Status: Needs work » Needs review

I'll let others chime in on documentation, I do not know :D

As it is, the MR is ready to review and get feedback from everyone

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work

Looking good, left a review. Haven't looked at the tests in detail, but they look really exhaustive, huge effort

pdureau’s picture

pdureau commented

Currently trying to get feedbacks from contrib module maintainers:

Icon packs "agnostic" modules:

Icon packs specific modules:

Service related module:

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Issue summary: View changes

Add link to #3482227: Support UI Icons to start listing some modules usage/integration.

mogtofu33’s picture

mogtofu33 commented
Status: Needs work » Needs review

Thank you @larowlan, for the initial code review.

The suggested changes have been merged, and most of the comments have been addressed. Only a few discussions remain.
The current failing functional test is not related to the Icon code but to the LayoutSectionTest. I'm unsure if we need to address this.

luenemann’s picture

luenemann
Primary language German
Location Südbaden, Germany
commented
Related issues: +#1849712: Add theming template and prepare logic for rendering icons

There is #1849712: Add theming template and prepare logic for rendering icons to merge Icon API module into core. I guess that could be closed as duplicate.

pdureau’s picture

pdureau commented

Hi Matthias (@luenemann),

Thanks for remembering us about #1849712: Add theming template and prepare logic for rendering icons I will add a comment.

https://www.drupal.org/project/icon last release was from March 2017, and last commit from June 2018. So, the proposal is a bit updated.

mogtofu33’s picture

mogtofu33 commented

Hi @quietone,

Thanks for the fixes and review!

I've improved the comment in `core/lib/Drupal/Core/Theme/Icon/Plugin/IconPackManager.php` to provide more clarity.

Additionally, I created a documentation page on the original UI Icons module: [here](https://project.pages.drupalcode.org/ui_icons/#examples). I will add this documentation (with some updates) into the official Drupal documentation once this merge is accepted.

Please let me know if you think it would be helpful to include further details in the comment, such as information on settings, template usage, or a preview mechanism.

mxh’s picture

mxh commented

"Extractors" remind me of plugin derivers. "Icon pack" reminds me of a base plugin ID. For me icons have characteristics of a (single-directory) component. Just a bit concerned that it might be over-complicated DX, especially when looking at already existing core concepts. In the end it's just about icons.

mxh’s picture

mxh commented

Another thought I had when looking at this: When any (imported/extracted/migrated?) icon is a plugin definition, then there may potentially be a lot of plugin definitions to be collected. From what I've seen on most sites, only a (very) small subset of icons of a library would be displayed on a website.

One example: many sites include Bootstrap icons or Google material icons as a whole library, but in the end they just use 10-20 of them in total, wheras the library itself comes with hundreds. There are other icon libraries around that may have thousands. If we're loading a plugin, then a default plugin manager will first load all existing plugin definitions (mostly from a cache backend) into memory and will keep them until the end of its service lifecycle. When only a small amount of the definitions are finally being used, this may be worth to be addressed for optimization (it's actually a potential problem for any sort of plugin manager).

finnsky’s picture

finnsky commented

While i research adaptation on https://www.drupal.org/project/drupal/issues/3483209 issue.
I found one big problem of this template approach.
We shouldn't define viewBox in template.
It is something that belong to svg itself and this attribute makes svg responsive.
Now it seems impossible to use different icon size on different breakpoints.

It is possible to use original viewBox from original SVG?

catch’s picture

catch
he/him
Primary language English
commented

Another thought I had when looking at this: When any (imported/extracted/migrated?) icon is a plugin definition, then there may potentially be a lot of plugin definitions to be collected. From what I've seen on most sites, only a (very) small subset of icons of a library would be displayed on a website.

Looking at the MR, each icon pack is a plugin definition, but every icon defined by the pack gets added to definitions. Then ::getIcons() gets an icon from the pack.

It would be possible to reduce the memory footprint on runtime by using a cache collector. Either not putting all the icons in the plugin definition, and getting them on demand (then caching the used ones), or adding an extra cache layer in front of the plugin manager that does this, so the plugin manager itself doesn't get consulted at all when frequently used icons are requested.

An example of a similar-ish pattern in core is Drupal\Core\Asset\LibraryDiscoveryCollector.

mogtofu33’s picture

mogtofu33 commented

@mxh, as highlighted by @catch, each icon is not a plugin; the icon pack itself is a plugin containing all extracted icons.

Note: We've used the term "Extractor" instead of "discovery" to avoid confusion with Drupal's core discovery mechanism. If someone have a better suggestion, please let me know.

The icon pack leverages the discovery mechanism to cache the list of available icons (DefaultPluginManager).

Current Behavior for IconPackManager works as follows:

  • Load Definitions: Loads icon definitions from *.icons.yml files.
  • Process Definitions: Runs the extractor plugin to generate a list of available icons for each definition.
  • Cache Definitions: Caches the processed definitions, including the icon list, in the discovery cache.

Later when getIcon is called:

  • Load Icon List: Retrieves the cached icon list from the discovery cache.
  • Find Matching Icon: Locates the requested icon (optimized for faster lookup on the first call).
  • Render Icon: Renders the icon using an inline inline_template based on the definition template.

Potential Optimization:

While we could optimize getIcon by caching a separate list of frequently used icons, the performance gain would be minimal, especially considering the relatively infrequent calls to getIcon and the efficiency of the current caching mechanism.

Even with multiple definitions and thousands of icons, the impact of extracting and rendering a few dozen icons on a page is negligible.

But I am open to any optimization, I will try to profile a second cache to get more specific numbers.

finnsky’s picture

finnsky commented

In addition to #57

Please check this comment https://www.drupal.org/project/drupal/issues/3483209#comment-15841982

pdureau’s picture

pdureau commented

@mxh, as highlighted by @catch, each icon is not a plugin; the icon pack itself is a plugin containing all extracted icons.

It is important that the icon pack is the plugin because, thanks to the YAML discovery, it allows front developer to declare icon packs in Drupal themes, in a friendly way.

The YAML formal was conceived for them, with the same mindset as the libraries.yml files and the SDC YAML format.

Note: We've used the term "Extractor" instead of "discovery" to avoid confusion with Drupal's core discovery mechanism. If someone have a better suggestion, please let me know.

Indeed, "Discovery" would be a confusing name. I would prefer to keep the existing name as status-quo, but I think "Collector", proposed by @catch, is as good as "Extractor" IMHO, because it conveys a similar meaning.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Thinking about cache collector it's definitely ++ to add IconPackCacheCollector like LibraryDiscoveryCollector moreover it should be enough to have API entry where contrib/custom code can filter out allowed icons from the whole set (shipped in a pack)

It will help to profile/trace more precise usage with less overhead meantime)

Looks like contrib can implement CSSClassExtractor and WebFontExtractor using this API, so that smells like ui_icons 3.x

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work

I think the suggestion to make use of a cache collector is a great idea. I think ::getIconsFromDefinition would likely be the best place to add it.

Setting to needs work for that

mogtofu33’s picture

mogtofu33 commented
Status: Needs work » Needs review

I added a cache collector for review.

In the meantime I fixed a memory footprint of loading thousands of icons in the pack definition, before each icon was an Object IconDefinition, now it's a simple array with minimal data. Plugin extractor must implement loadIcon()to return the object which is a big improvement on the cache bin size, around 8 times smaller.

pdureau’s picture

pdureau commented
Assigned: Unassigned » larowlan
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned

Left a review yesterday

mogtofu33’s picture

mogtofu33 commented
Assigned: Unassigned » mogtofu33
Status: Needs review » Needs work
mogtofu33’s picture

mogtofu33 commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Thanks @mogtofu33 - I think this is ready now. Did we get everything perfect? It's hard to know until people start using it, and they can't start using it until we get it in. So marking this as RTBC. The new APIs are marked experimental so we have that to allow some iteration as we find things.

I think the last task here is a change record. I will start on that

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: -Needs change record

Added a change record draft at https://www.drupal.org/node/3490350, please review and refine as needed.

pdureau’s picture

pdureau commented
Assigned: mogtofu33 » Unassigned

Great news @larowlan

We (Jean & me) updated the change notice: https://www.drupal.org/node/3490350
And we have added some credits.

So, everything looks ready now. The merge is the next step?

johnwebdev’s picture

johnwebdev commented

I would love to see an simple example EXTENSION_NAME.icons.yml in the change record

The extractor example file is quite overwhelming

pdureau’s picture

pdureau commented

I would love to see an simple example EXTENSION_NAME.icons.yml in the change record
The extractor example file is quite overwhelming

The examples in the change records are EXTENSION_NAME.icons.yml.

Indeed, we have shown complex stuff to display the power of the API. We will see if we can show something simpler.

mogtofu33’s picture

mogtofu33 commented

Hi @johnwebdev, change records updated with more simple examples, let me know if it's more clear.

catch’s picture

catch
he/him
Primary language English
commented

Moving this to the issue since the thread on the MR is closed and I don't want to re-open it.

I don't really like the fact that getIcon and getIcons have different purpose now but seems so close, getIcons, as before, should logically return an array of Icon objects IconDefinition but not anymore because with thousands of icons the memory footprint was too important. It only return a minimal needed list of icons data that then need to be loaded if displayed. So perhaps it make sense to change naming here...

I'm wondering if the 'get all icons' case could be done with an iterator + array access, re-using the cache collector but allowing for foreach() too. However if we decide we need/want that, then we should be able to just add a getAllIcons() or getIconIterator() method or something so doesn't need to block anything here.

Side note to discuss: Because the icon cache collector grow over time, this is where I need to be careful with ui_icons module implementation of FormElement autocomplete, picker and library as if I do a getIcon on abig list, the cache data will grow very quickly.

This could probably be handled by varying the cache entry by user permissions or $is_admin_route or similar - could open a follow-up to look at it. I think we already do this in one or two cache collectors for similar reasons maybe.

catch’s picture

catch
he/him
Primary language English
commented

First of all looking into that made me realize I wrongly used the CacheCollector and was missing a proper set. It's fixed and the collector works as designed (I hope so, must admit I struggled a bit).

I double checked here and made one extra change.

The cache collector service was missing the needs_destruction tag. When it has that, the ::destruct() method is called during kernel destruction and this handles merging anything in the cache backend, with any new keys that are set and flagged to persist in the cache.

What this allows for is a single cache set after the response is sent to the browser, instead of one for each time there's a cache miss. It also means that every time the cache is set, it's additive to any other cache sets that happened during the request (e.g. stampede situations), because we get and merge before writing back to the cache.

I just re-reviewed this again since it was a while since last time, and couldn't find anything else to complain about except one documentation nit.

Except there is a unit test failure... asking in slack about that.

There are a few things I am not fully confident about but that is unfamiliarity with the contrib module/use cases probably - like @larowlan said we can find issue by trying to use it, and fix them easily enough because it's @internal.

  • catch committed 95350475 on 11.1.x 
    Issue #3471494 by mogtofu33, catch, pdureau, nod_, larowlan, andypost,...

  • catch committed ed6e9292 on 11.x 
    Issue #3471494 by mogtofu33, catch, pdureau, nod_, larowlan, andypost,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

The unit test failure was a kernel test in the wrong namespace. No database on the unit test images so it shows up there but not if you run locally.

I had make the diff to apply locally with --binary because .diff in the gitlab MR UI does not use that. I hope I did this correctly :/ Looks right though.

Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hi,

Congratulations for the merge!!! \o/

Something I thought about the change record: maybe it is ok to add a link to:
- https://www.drupal.org/project/ui_icons to give people way to use it in site building
- and/or a link to https://gitlab.com/ui-icons/ui-icons-example for advanced more exhaustive examples?
- and/or a link to the test module https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste... for the examples of declaration?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

One of the tests here appears to have broken git checkout (and possibly tar/unzip) on Windows.

Via @spokje in #3490163-16: Update Composer dependencies for 11.1.0:

$ git checkout -b '3490163-11.x-bump-composer-dependencies' --track drupal-3490163/'3490163-11.x-bump-composer-dependencies'
error: invalid path 'core/modules/system/tests/modules/icon_test/icons/name_special_chars/FoO !?1:èç 2 "#3 B*;**a,ù$R|~¹&{[].svg'
mogtofu33’s picture

mogtofu33 commented

@catch, thanks for the review, cache collector fixes and merge.

@grimreaper, good idea, added link to ui_icons and tests. I think relate to an external repository for examples should be avoided.

  • catch committed d253ca7d on 11.1.x 
    Issue #3471494 by catch: follow-up commit to remove icon with special...

  • catch committed 25ac783c on 11.x 
    Issue #3471494 by catch: follow-up commit to remove icon with special...
catch’s picture

catch
he/him
Primary language English
commented

I've removed the special chars test coverage and related test icon. Would be good if someone could open a follow-up to add it back with special chars that are still valid on windows filesystems.

quietone’s picture

quietone commented
Issue tags: +Needs release note
catch’s picture

catch
he/him
Primary language English
commented

I personally wouldn't add a release note here because this doesn't affect existing sites at all.

We could put it in the release highlights though?

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Issue tags: +11.1.0 release highlights
quietone’s picture

quietone commented
Issue tags: -Needs release note

#87 makes sense, removing tag.

mogtofu33’s picture

mogtofu33 commented

Hello all, here is an highlight proposition:

A new API has been added to Drupal, empowering you to seamlessly integrate custom or third-party icon packs!
This powerful and flexible API allows modules and themes to define icon packs using the YAML plugin discovery system (EXTENSION_NAME.icons.yml).
By extracting the icon list from each pack, the API exposes them through the versatile icon render element and the convenient icon() Twig function.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.