Skip to content

[Icons] Do not fail application if there is not internet connection #3146

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
+21 −0
Open

[Icons] Do not fail application if there is not internet connection #3146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions src/Icons/src/Twig/UXIconRuntime.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\UX\Icons\Twig;

use Psr\Log\LoggerInterface;
use Symfony\Component\HttpClient\Exception\TransportException;
use Symfony\UX\Icons\Exception\IconNotFoundException;
use Symfony\UX\Icons\IconRendererInterface;
use Twig\Extension\RuntimeExtensionInterface;
Expand Down Expand Up @@ -45,6 +46,10 @@ public function renderIcon(string $name, array $attributes = []): string
}

throw $e;
} catch (TransportException $e) {
$this->logger?->warning($e->getMessage());

return '';
}
Comment on lines +49 to 53
Copy link
Member

@Kocal Kocal Oct 20, 2025

Choose a reason for hiding this comment

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

We could merge this block with the catch (IconNotFoundException $e)

Copy link
Author

@Zales0123 Zales0123 Oct 21, 2025

Choose a reason for hiding this comment

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

The question is - should it be handled only if the ignoreNotFound flag is set to true, or it should work this way no matter what 🤔

Copy link
Contributor

@Seb33300 Seb33300 Oct 21, 2025

Choose a reason for hiding this comment

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

should it be handled only if the ignoreNotFound flag is set to true

IMO, yes

Copy link
Author

@Zales0123 Zales0123 Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Up to you, @Kocal 🖖 From my perspetive, a decision of not failing on not found icons is a consious choice: "if I define an unknown icon it should not fail" and in fact is an opt-in. On the other hand, not failing the application when something completely independent of the developer happens (like the internet connection problems) should be a default behaviour (or maybe opt-out 🤷).

Anyway, if you decide to cover this with the existing flag-based approach, I'm ok with that 🫡 but maybe in such a situation we should indeed just handle the TransportException in IconifyOnDemandRegistry and throw IconNotFoundException so no changes in the runtime service is needed. Again - up to you, I'm just a humble developer that found a bug 😄

Copy link
Member

@Kocal Kocal Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion I find it ridiculous that an app could returns a 500 if a single icon was not able to be downloaded on-the-fly (implying the icon was not imported/locked during development or deployment).

Your addition is nice (we still need to merge the 2 catch), however I think the DX could be improved.
I wasn't aware about this option, and I think some people too, but IMHO it would be nice to add a new Symfony Recipe for UX Icon with:

  • the option set to false true, so we make it explicit
  • the option set to true false in when@dev

Do you want to work on it? I can help you if you need.

Thanks!

Copy link
Contributor

@Seb33300 Seb33300 Oct 22, 2025

Choose a reason for hiding this comment

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

the option set to false, so we make it explicit
the option set to true in when@dev

IMO, ignoreNotFound should be true to prevent production to return a 500 when an icon is missing.
And false in when@dev

This is why that option was originally added: #2008

Copy link
Member

@Kocal Kocal Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes indeed, I forgot what was the option name, but yes these errors should be silent in prod. I updated my message.

}

Expand Down
16 changes: 16 additions & 0 deletions src/Icons/tests/Unit/Twig/UXIconRuntimeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
use Symfony\Component\HttpClient\Exception\TransportException;
use Symfony\UX\Icons\Exception\IconNotFoundException;
use Symfony\UX\Icons\IconRendererInterface;
use Symfony\UX\Icons\Twig\UXIconRuntime;
Expand Down Expand Up @@ -40,4 +41,19 @@ public function testRenderIconIgnoreNotFound()
$this->expectException(IconNotFoundException::class);
$runtime->renderIcon('not_found');
}

public function testRenderIconReturnsEmptyStringIfConnectionIsNotPossible()
{
$renderer = $this->createMock(IconRendererInterface::class);
$renderer->method('renderIcon')
->willThrowException(new TransportException('Could not resolve host api.iconify.design'));

$logger = $this->createMock(LoggerInterface::class);
$logger->expects($this->once())
->method('warning')
->with('Could not resolve host api.iconify.design');

$runtime = new UXIconRuntime($renderer, true, $logger);
$this->assertEquals('', $runtime->renderIcon('tabler:search'));
}
}
Loading