Skip to content

[Icons] Add ignore_not_found config option #2023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Icons/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# CHANGELOG

## 2.19.0

- Add `ignore_not_found` option to silence error during rendering if the
icon is not found.

## 2.17.0

- Add component
3 changes: 2 additions & 1 deletion src/Icons/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
"symfony/http-client": "6.4|^7.0",
"symfony/phpunit-bridge": "^6.3|^7.0",
"symfony/ux-twig-component": "^2.14",
"zenstruck/console-test": "^1.5"
"zenstruck/console-test": "^1.5",
"psr/log": "^2|^3"
},
"config": {
"sort-packages": true
Expand Down
10 changes: 9 additions & 1 deletion src/Icons/config/services.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Symfony\UX\Icons\Registry\LocalSvgIconRegistry;
use Symfony\UX\Icons\Twig\IconFinder;
use Symfony\UX\Icons\Twig\UXIconExtension;
use Symfony\UX\Icons\Twig\UXIconRuntime;

return static function (ContainerConfigurator $container): void {
$container->services()
Expand All @@ -44,11 +45,18 @@
->set('.ux_icons.twig_icon_extension', UXIconExtension::class)
->tag('twig.extension')

->set('.ux_icons.twig_icon_runtime', UXIconRuntime::class)
->args([
service('.ux_icons.icon_renderer'),
abstract_arg('ignore_not_found'),
service('logger')->ignoreOnInvalid(),
])
->tag('twig.runtime')

->set('.ux_icons.icon_renderer', IconRenderer::class)
->args([
service('.ux_icons.icon_registry'),
])
->tag('twig.runtime')

->alias('Symfony\UX\Icons\IconRendererInterface', '.ux_icons.icon_renderer')

Expand Down
16 changes: 16 additions & 0 deletions src/Icons/doc/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,19 @@ Now, all icons will have the ``fill`` attribute set to ``currentColor`` by defau
# renders "user-profile.svg" with fill="red"
{{ ux_icon('user-profile', {fill: 'red'}) }}

Errors
------

If an icon is not found, an exception is thrown. This is useful during development,
but in production, you may want to render an error message instead. You can do this
by setting the ``ignore_not_found`` configuration option to ``true``:

.. code-block:: yaml

# config/packages/ux_icons.yaml
ux_icons:
ignore_not_found: true

Accessibility
-------------

Expand Down Expand Up @@ -515,6 +528,9 @@ Full Configuration

# The endpoint for the Iconify API.
endpoint: 'https://api.iconify.design'

# Whether to ignore errors when an icon is not found.
ignore_not_found: false

Learn more
----------
Expand Down
10 changes: 9 additions & 1 deletion src/Icons/src/DependencyInjection/UXIconsExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ public function getConfigTreeBuilder(): TreeBuilder
->end()
->end()
->end()
->booleanNode('ignore_not_found')
->info('Ignore error when an icon is not found.')
->defaultFalse()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could default to %kernel.debug% here or in the recipe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea in #2008 was to default to "not kernel.debug".

I this change anything in prod, in would be a BC break, so maybe the recipe is a good solution, indeed

->end()
->end()
;

Expand All @@ -69,7 +73,7 @@ public function getConfiguration(array $config, ContainerBuilder $container): Co
return $this;
}

protected function loadInternal(array $mergedConfig, ContainerBuilder $container): void // @phpstan-ignore-line
protected function loadInternal(array $mergedConfig, ContainerBuilder $container): void
{
$loader = new PhpFileLoader($container, new FileLocator(__DIR__.'/../../config'));
$loader->load('services.php');
Expand All @@ -96,6 +100,10 @@ protected function loadInternal(array $mergedConfig, ContainerBuilder $container
->setArgument(1, $mergedConfig['default_icon_attributes'])
;

$container->getDefinition('.ux_icons.twig_icon_runtime')
->setArgument(1, $mergedConfig['ignore_not_found'])
;

if ($mergedConfig['iconify']['enabled']) {
$loader->load('iconify.php');

Expand Down
3 changes: 1 addition & 2 deletions src/Icons/src/Twig/UXIconExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Symfony\UX\Icons\Twig;

use Symfony\UX\Icons\IconRenderer;
use Twig\Extension\AbstractExtension;
use Twig\TwigFunction;

Expand All @@ -25,7 +24,7 @@ final class UXIconExtension extends AbstractExtension
public function getFunctions(): array
{
return [
new TwigFunction('ux_icon', [IconRenderer::class, 'renderIcon'], ['is_safe' => ['html']]),
new TwigFunction('ux_icon', [UXIconRuntime::class, 'renderIcon'], ['is_safe' => ['html']]),
];
}
}
49 changes: 49 additions & 0 deletions src/Icons/src/Twig/UXIconRuntime.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\UX\Icons\Twig;

use Psr\Log\LoggerInterface;
use Symfony\UX\Icons\Exception\IconNotFoundException;
use Symfony\UX\Icons\IconRendererInterface;
use Twig\Extension\RuntimeExtensionInterface;

/**
* @author Simon André <[email protected]>
*
* @internal
*/
final class UXIconRuntime implements RuntimeExtensionInterface
{
public function __construct(
private readonly IconRendererInterface $iconRenderer,
private readonly bool $ignoreNotFound = false,
private readonly ?LoggerInterface $logger = null,
) {
}

/**
* @param array<string, bool|string> $attributes
*/
public function renderIcon(string $name, array $attributes = []): string
{
try {
return $this->iconRenderer->renderIcon($name, $attributes);
} catch (IconNotFoundException $e) {
if ($this->ignoreNotFound) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's a Twig rendering context, I'm wondering if we should use Environment::isStrictVariables() instead. I can't picture a case where the twig.strict_variables config is different from ux_icons.ignoreNotFound. They serve the same purpose for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this also, at least until we're asked for a separate config option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If by "using Environment::isStrictVariables()" you mean "do not throw if strictVariable = false", this would be a BC break, no ?

Copy link
Member

@yceruto yceruto Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not throwing the exception in the prod env doesn't seem like a critical behavior change to me. In fact, that's exactly what the reporter is requesting (aligned with how Twig behaves). As suggested #2023 (comment) we could log the error instead.

The configs (strict_variables and ignore_not_found) appear to be opposite due to their naming, but they actually represent the same concept/purpose.

$this->logger?->warning($e->getMessage());
return '';
}

throw $e;
}
}
}
43 changes: 43 additions & 0 deletions src/Icons/tests/Unit/Twig/UXIconRuntimeTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <[email protected]>
*
* 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\Twig;

use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
use Symfony\UX\Icons\Exception\IconNotFoundException;
use Symfony\UX\Icons\IconRendererInterface;
use Symfony\UX\Icons\Twig\UXIconRuntime;

/**
* @author Simon André <[email protected]>
*/
class UXIconRuntimeTest extends TestCase
{
public function testRenderIconIgnoreNotFound(): void
{
$renderer = $this->createMock(IconRendererInterface::class);
$renderer->method('renderIcon')
->willThrowException(new IconNotFoundException('Icon "foo" not found.'));

$logger = $this->createMock(LoggerInterface::class);
$logger->expects($this->once())
->method('warning')
->with('Icon "foo" not found.');

$runtime = new UXIconRuntime($renderer, true, $logger);
$this->assertEquals('', $runtime->renderIcon('not_found'));

$runtime = new UXIconRuntime($renderer, false);
$this->expectException(IconNotFoundException::class);
$runtime->renderIcon('not_found');
}
}