Repository: symfony/ux
Pull request #1594 (2024-03-09 01:22:10)

[Icons] Improve cache warmup by looking at every string
| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Issues        | n/a
| License       | MIT

Improve working with dynamic icon names (inspired by https://v2.tailwindcss.com/docs/optimizing-for-production#writing-purgeable-html)

```twig
{# This will be cached #}
{{ ux_icon('flag-fr') }}

{# This will NOT be cached #}
{{ ux_icon('flag-' ~ locale) }}

{% set flags = {fr: 'flag-fr', de: 'flag-de'} %} {# both "flag-fr" and "flag-de" will be cached #}
{{ ux_icon(flags[locale]) }}
```

Closes #1601

kbond: @smnandre made some fair points about performance/security.

Regarding security: I want to clarify that only strings that match `'foo:bar'` will be used to make http requests to iconify. `'foo:bar'` will create an http request with this value in the request path but `'something-sensitive'` will not. However, `'something:sensitive'` would create an http request with this value in the request path.

I'd like to find a compromise that we're all ok with, some thoughts:
- opt-in to this much looser string check
- configure specific paths/globs (like tailwind does)
smnandre: We can add a failsafe: store locally the list of available icon prefixes. 

Then we reverse the logic: 
* collect all the icons as you do now
* list the prefixes
* download the list of icons for each prefix

--> finally download the icons.

It can be even faster in the end, because we can group parallelize + group icon downloads (up to 500 per request)
smnandre: I'm reading the doc 

> During development, if you modify an icon, you will need to clear the cache (``bin/console cache:clear``) to see the changes.
    
Could we add a isFresh / mtime check about that ? What do you think ? 
kbond: I believe the security concerns have been alleviated. I now only make the http request if the prefix is valid. `'something:sensitive'` will no longer create a request.
smnandre: I love this!

Patch:
diff --git a/src/Icons/config/iconify.php b/src/Icons/config/iconify.php
index 70c795ca..7ab0019d 100644
--- a/src/Icons/config/iconify.php
+++ b/src/Icons/config/iconify.php
@@ -25,6 +25,7 @@ return static function (ContainerConfigurator $container): void {
 
         ->set('.ux_icons.iconify', Iconify::class)
             ->args([
+                service('cache.system'),
                 abstract_arg('endpoint'),
                 service('http_client')->nullOnInvalid(),
             ])
diff --git a/src/Icons/doc/index.rst b/src/Icons/doc/index.rst
index d34fda7c..40423628 100644
--- a/src/Icons/doc/index.rst
+++ b/src/Icons/doc/index.rst
@@ -115,6 +115,22 @@ In production, you can pre-warm the cache by running the following command:
 
 This command looks in all your twig templates for ``ux_icon`` calls and caches the icons it finds.
 
+.. caution::
+
+    Icons that have a name built dynamically will not be cached. It's advised to have the icon
+    name as a string literal in your templates.
+
+    .. code-block:: twig
+
+        {# This will be cached #}
+        {{ ux_icon('flag-fr') }}
+
+        {# This will NOT be cached #}
+        {{ ux_icon('flag-' ~ locale) }}
+
+        {% set flags = {fr: 'flag-fr', de: 'flag-de'} %} {# both "flag-fr" and "flag-de" will be cached #}
+        {{ ux_icon(flags[locale]) }}
+
 .. note::
 
     During development, if you modify an icon, you will need to clear the cache (``bin/console cache:clear``)
@@ -142,9 +158,6 @@ Full Default Configuration
         iconify:
            enabled:              true
 
-           # Whether to use the "on demand" icons powered by Iconify.design.
-           on_demand:            true
-
            # The endpoint for the Iconify API.
            endpoint:             'https://api.iconify.design'
 
diff --git a/src/Icons/src/Command/WarmCacheCommand.php b/src/Icons/src/Command/WarmCacheCommand.php
index 3c4fb136..548c6f3c 100644
--- a/src/Icons/src/Command/WarmCacheCommand.php
+++ b/src/Icons/src/Command/WarmCacheCommand.php
@@ -45,7 +45,6 @@ final class WarmCacheCommand extends Command
                     $io->writeln(sprintf(' Warmed icon <comment>%s</comment>.', $name));
                 }
             },
-            onFailure: fn (string $name) => $io->warning(sprintf('Icon <comment>%s</comment> not found.', $name))
         );
 
         $io->success('Icon cache warmed.');
diff --git a/src/Icons/src/DependencyInjection/UXIconsExtension.php b/src/Icons/src/DependencyInjection/UXIconsExtension.php
index 9100cbed..fe0c3727 100644
--- a/src/Icons/src/DependencyInjection/UXIconsExtension.php
+++ b/src/Icons/src/DependencyInjection/UXIconsExtension.php
@@ -46,10 +46,6 @@ final class UXIconsExtension extends ConfigurableExtension implements Configurat
                     ->info('Configuration for the "on demand" icons powered by Iconify.design.')
                     ->{interface_exists(HttpClientInterface::class) ? 'canBeDisabled' : 'canBeEnabled'}()
                     ->children()
-                        ->booleanNode('on_demand')
-                            ->info('Whether to use the "on demand" icons powered by Iconify.design.')
-                            ->defaultTrue()
-                        ->end()
                         ->scalarNode('endpoint')
                             ->info('The endpoint for the Iconify API.')
                             ->defaultValue('https://api.iconify.design')
@@ -95,12 +91,8 @@ final class UXIconsExtension extends ConfigurableExtension implements Configurat
             $loader->load('iconify.php');
 
             $container->getDefinition('.ux_icons.iconify')
-                ->setArgument(0, $mergedConfig['iconify']['endpoint'])
+                ->setArgument(1, $mergedConfig['iconify']['endpoint'])
             ;
-
-            if (!$mergedConfig['iconify']['on_demand']) {
-                $container->removeDefinition('.ux_icons.iconify_on_demand_registry');
-            }
         }
 
         if (!$container->getParameter('kernel.debug')) {
diff --git a/src/Icons/src/Iconify.php b/src/Icons/src/Iconify.php
index 2dd979a6..4899c4c6 100644
--- a/src/Icons/src/Iconify.php
+++ b/src/Icons/src/Iconify.php
@@ -14,6 +14,7 @@ namespace Symfony\UX\Icons;
 use Symfony\Component\HttpClient\Exception\JsonException;
 use Symfony\Component\HttpClient\HttpClient;
 use Symfony\Component\HttpClient\ScopingHttpClient;
+use Symfony\Contracts\Cache\CacheInterface;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\UX\Icons\Exception\IconNotFoundException;
 use Symfony\UX\Icons\Svg\Icon;
@@ -26,8 +27,10 @@ use Symfony\UX\Icons\Svg\Icon;
 final class Iconify
 {
     private HttpClientInterface $http;
+    private \ArrayObject $sets;
 
     public function __construct(
+        private CacheInterface $cache,
         string $endpoint = 'https://api.iconify.design',
         ?HttpClientInterface $http = null,
     ) {
@@ -40,13 +43,15 @@ final class Iconify
 
     public function metadataFor(string $prefix): array
     {
-        $response = $this->http->request('GET', sprintf('/collections?prefix=%s', $prefix));
-
-        return $response->toArray()[$prefix] ?? throw new \RuntimeException(sprintf('The icon prefix "%s" does not exist on iconify.design.', $prefix));
+        return $this->sets()[$prefix] ?? throw new \RuntimeException(sprintf('The icon prefix "%s" does not exist on iconify.design.', $prefix));
     }
 
     public function fetchIcon(string $prefix, string $name): Icon
     {
+        if (!isset($this->sets()[$prefix])) {
+            throw new IconNotFoundException(sprintf('The icon "%s:%s" does not exist on iconify.design.', $prefix, $name));
+        }
+
         $response = $this->http->request('GET', sprintf('/%s.json?icons=%s', $prefix, $name));
 
         try {
@@ -66,6 +71,10 @@ final class Iconify
 
     public function fetchSvg(string $prefix, string $name): string
     {
+        if (!isset($this->sets()[$prefix])) {
+            throw new IconNotFoundException(sprintf('The icon "%s:%s" does not exist on iconify.design.', $prefix, $name));
+        }
+
         $content = $this->http
             ->request('GET', sprintf('/%s/%s.svg', $prefix, $name))
             ->getContent()
@@ -77,4 +86,13 @@ final class Iconify
 
         return $content;
     }
+
+    private function sets(): \ArrayObject
+    {
+        return $this->sets ??= $this->cache->get('ux-iconify-sets', function () {
+            $response = $this->http->request('GET', '/collections');
+
+            return new \ArrayObject($response->toArray());
+        });
+    }
 }
diff --git a/src/Icons/src/Registry/InMemoryIconRegistry.php b/src/Icons/src/Registry/InMemoryIconRegistry.php
deleted file mode 100644
index 8f9ad129..00000000
--- a/src/Icons/src/Registry/InMemoryIconRegistry.php
+++ /dev/null
@@ -1,40 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\UX\Icons\Registry;
-
-use Symfony\UX\Icons\Exception\IconNotFoundException;
-use Symfony\UX\Icons\IconRegistryInterface;
-use Symfony\UX\Icons\Svg\Icon;
-
-/**
- * @author Simon André <smn.andre@gmail.com>
- */
-final class InMemoryIconRegistry implements IconRegistryInterface
-{
-    public function __construct(
-        /**
-         * @var array<string, Icon> $icons
-         */
-        private array $icons = [],
-    ) {
-    }
-
-    public function set(string $name, Icon $icon): void
-    {
-        $this->icons[$name] = $icon;
-    }
-
-    public function get(string $name): Icon
-    {
-        return $this->icons[$name] ?? throw new IconNotFoundException(sprintf('Icon "%s" not found.', $name));
-    }
-}
diff --git a/src/Icons/src/Twig/IconFinder.php b/src/Icons/src/Twig/IconFinder.php
index 4411e5c7..f6f1940c 100644
--- a/src/Icons/src/Twig/IconFinder.php
+++ b/src/Icons/src/Twig/IconFinder.php
@@ -38,11 +38,7 @@ final class IconFinder
         foreach ($this->files($this->twig->getLoader()) as $file) {
             $contents = file_get_contents($file);
 
-            if (preg_match_all('#ux_icon\(["\']([\w:-]+)["\']#', $contents, $matches)) {
-                $found[] = $matches[1];
-            }
-
-            if (preg_match_all('#name=["\']([\w:-]+)["\']#', $contents, $matches)) {
+            if (preg_match_all('#["\']([\w:-]+)["\']#', $contents, $matches)) {
                 $found[] = $matches[1];
             }
         }


Test patch:
diff --git a/src/Icons/tests/Fixtures/TestKernel.php b/src/Icons/tests/Fixtures/TestKernel.php
index acee6579..fa1ddf69 100644
--- a/src/Icons/tests/Fixtures/TestKernel.php
+++ b/src/Icons/tests/Fixtures/TestKernel.php
@@ -1,14 +1,5 @@
 <?php
 
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
 namespace Symfony\UX\Icons\Tests\Fixtures;
 
 use Psr\Log\NullLogger;
@@ -35,9 +26,9 @@ final class TestKernel extends Kernel
         yield new UXIconsBundle();
     }
 
-    protected function configureContainer(ContainerConfigurator $container): void
+    protected function configureContainer(ContainerConfigurator $c): void
     {
-        $container->extension('framework', [
+        $c->extension('framework', [
             'secret' => 'S3CRET',
             'test' => true,
             'router' => ['utf8' => true],
@@ -46,22 +37,21 @@ final class TestKernel extends Kernel
             'php_errors' => ['log' => true],
             'property_access' => true,
             'http_client' => true,
-            'handle_all_throwables' => true,
         ]);
 
-        $container->extension('twig', [
-            'default_path' => __DIR__ . '/templates',
+        $c->extension('twig', [
+            'default_path' => __DIR__.'/templates',
         ]);
 
-        $container->extension('twig_component', [
+        $c->extension('twig_component', [
             'defaults' => [],
             'anonymous_template_directory' => 'components',
         ]);
 
-        $container->extension('ux_icons', [
+        $c->extension('ux_icons', [
             'icon_dir' => '%kernel.project_dir%/tests/Fixtures/icons',
         ]);
 
-        $container->services()->set('logger', NullLogger::class);
+        $c->services()->set('logger', NullLogger::class);
     }
 }
diff --git a/src/Icons/tests/Fixtures/templates/template2.html.twig b/src/Icons/tests/Fixtures/templates/template2.html.twig
index 859dcabf..6ddc6732 100644
--- a/src/Icons/tests/Fixtures/templates/template2.html.twig
+++ b/src/Icons/tests/Fixtures/templates/template2.html.twig
@@ -1,3 +1,5 @@
+{% set icon = 'flag:eu-4x3' %}
+
 {{ ux_icon('something:invalid') }}
 {{ ux_icon('invalid') }}
 {{ ux_icon('iconamoon:invalid') }}
diff --git a/src/Icons/tests/Integration/Command/WarmCacheCommandTest.php b/src/Icons/tests/Integration/Command/WarmCacheCommandTest.php
index 5e53db21..668bcbcd 100644
--- a/src/Icons/tests/Integration/Command/WarmCacheCommandTest.php
+++ b/src/Icons/tests/Integration/Command/WarmCacheCommandTest.php
@@ -29,6 +29,7 @@ final class WarmCacheCommandTest extends KernelTestCase
             ->assertOutputContains('Warmed icon user.')
             ->assertOutputContains('Warmed icon sub:check.')
             ->assertOutputContains('Warmed icon iconamoon:3d-duotone.')
+            ->assertOutputContains('Warmed icon flag:eu-4x3.')
             ->assertOutputContains('Icon cache warmed.')
         ;
     }
diff --git a/src/Icons/tests/Unit/Registry/InMemoryIconRegistryTest.php b/src/Icons/tests/Unit/Registry/InMemoryIconRegistryTest.php
deleted file mode 100644
index 49635e41..00000000
--- a/src/Icons/tests/Unit/Registry/InMemoryIconRegistryTest.php
+++ /dev/null
@@ -1,56 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\UX\Icons\Tests\Unit\Registry;
-
-use PHPUnit\Framework\TestCase;
-use Symfony\UX\Icons\Exception\IconNotFoundException;
-use Symfony\UX\Icons\Registry\InMemoryIconRegistry;
-use Symfony\UX\Icons\Svg\Icon;
-
-/**
- * @author Simon André <smn.andre@gmail.com>
- */
-final class InMemoryIconRegistryTest extends TestCase
-{
-    public function testRegistryConstructor(): void
-    {
-        $icon = new Icon('foo', ['bar' => 'foobar']);
-        $registry = new InMemoryIconRegistry(['foo' => $icon]);
-
-        $this->assertSame($icon, $registry->get('foo'));
-    }
-
-    public function testRegistryReplaceIcon(): void
-    {
-        $registry = new InMemoryIconRegistry();
-        $foo = new Icon('foo', []);
-        $bar = new Icon('bar', []);
-
-        $registry->set('foo', $foo);
-        $this->assertSame($foo, $registry->get('foo'));
-
-        $registry->set('bar', $bar);
-        $this->assertSame($bar, $registry->get('bar'));
-
-        $registry->set('bar', $foo);
-        $this->assertSame($foo, $registry->get('bar'));
-    }
-
-    public function testRegistryThrowsExceptionOnUnknownIcon(): void
-    {
-        $this->expectException(IconNotFoundException::class);
-        $this->expectExceptionMessage('Icon "foo" not found.');
-
-        $registry = new InMemoryIconRegistry();
-        $registry->get('foo');
-    }
-}