Add an icon management API

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

Some typos

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

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.

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.

It could take weeks looking at 💬 Offer to maintain Icon API Active

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 📌 Ensure that Book does not get special core treatment Needs review . 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.

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).

I assign the issue to me, to work with mogtofu33 →

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

• the mandatory stuff from ui_icons main module:
• 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

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

A few notes.

Decisions

We will wait ui_icons → 1.0.0-beta2 before 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) :

We will also propose path extractor in the MR, following Jean ( mogtofu33 → ) feedback:

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 , I think path is better and lighter for clean svgs pack (not mixed size/viewport).

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\???

Drupal\ui_icon

Now that this is going into core/lib/Drupal/Core, it should be Drupal\Icon or Drupal\Icons - snake case is only used for modules.

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.

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.

We decided during the Barcelona meeting to not make this API a Drupal\Core sub-system instead of an experimental module. One of the reasons was to not experience again the hassle of 📌 Move code from the experimental SDC module to core Fixed

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

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...

Some thoughts to share before the MR

With this Icons API (UI Icons now, Core later) 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.

In the issue summary, there were 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 .

The MR will be the proposal 1, so the form element(s) will be absent and could be proposed in a distinct MR later.

🎉

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

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

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.

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.

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.

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

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.

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.

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

Right, there would be no version control if icons are entities.

It would need to 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?

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

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

Indeed. 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 in content (ex: Field API), config (ex: 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 the design system are shippable in a single unit of implementation: the Drupal theme. You can think about this API as "the SDC of icons". So, it is definitely code.

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.

@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 is in there will be work to make navigation api use it it'll be clearer then.

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.

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.

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.

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

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

Currently trying to onboard more module maintainers:

"Agnostic" modules:

• iconset: 💬 Thanks you for this wonderful module Active
• icons: 🌱 Compatibility with the Icon API proposal for Core Active
• ex_icons: 🌱 Compatibility with the Icon API proposal for Core Active

Icon packs specific modules:

• material_icons: 🌱 Compatibility with the Icon API proposal for Core Active
• fontawesome: TODO
• menu_bootstrap_icon: TODO
• micon: TODO
• md_fontello: TODO

I will udpate the list soon.

Add link to ✨ Support UI Icons Active to start listing some modules usage/integration.

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.

There is ✨ Add theming template and prepare logic for rendering icons Active to merge Icon API module → into core. I guess that's could be closed as duplicate.

Hi Matthias (@luenemann),

Thanks for remembering us about ✨ Add theming template and prepare logic for rendering icons Active 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.

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.

"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 atalready existing core concepts. In the end it's just about icons.

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.

While i research adaptation on https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work 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.

It is possible to use original viewBox from original SVG?

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.

@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.

In addition to #57

Please check this comment https://www.drupal.org/project/drupal/issues/3483209#comment-15841982 📌 Navigation leverage icon core API Needs work

@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.

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

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

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.

Left a review yesterday

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

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

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?

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

The extractor example file is quite overwhelming

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.

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

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.

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.

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!

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?

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-RC1 → :

$ 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'

@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.

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.

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?

#87 makes sense, removing tag.

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.

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