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

[Zales0123]

Q	A
Bug fix?	yes
New feature?	no
Deprecations?	no
Documentation?	no
Issues
License	MIT

Hello 👋 First of all, thank you for amazing SymfonyUX initiative, it definitely brings a frash air in working with front-end for a total FE noobs like me 😅

I've recently encountered a rather unusual problem while playing in icons, still a pretty annoying one. I was coding in a plane and for some reason decided clear my cache. It resulted in a situation, when I had no icons information loaded in cache and they could not be fetched from the remote server as I had no internet connection ✈️ It resulted in 500 error and not being able to use my application just because of the missing icons 😅 I believe such a situation should be handled by default and unablity to show icons should not break the application.

I propose the easiest possible solution - catching the TransportException on the high level in the UXIconRuntime. However, I suppose it could be done nicer, to not expose any registry specific information (in the end, only the IconifyOnDemandRegistry could result in such error) - maybe we should handle it in the registry itself, and throw some more generic exception, like IconsNotAvailableException? Or maybe even use the existing IconNotFoundException? 🤔 I didn't want to push any of these solutions, and would prefer to see the opinion of the library maintainers 🫡

[Kocal]

Hi Mateusz, by any chance were you at the Sylius Con in Lyon this last friday? :)

About your changes, I already faced the same issue when working on a train/plane, where the connection was really bad and icons were not locked (ie #3084).

[Kocal]

CI failures seem unrelated

EDIT: posted at zenstruck/foundry#1011 (comment)

[Kocal]

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

[Zales0123]

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 🤔

[Seb33300]

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

IMO, yes

[Zales0123]

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 😄

[Kocal]

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!

[Seb33300]

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

[Kocal]

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

[Zales0123]

Hi Mateusz, by any chance were you at the Sylius Con in Lyon this last friday? :)

Indeed I was there 😅

CI failures seem unrelated

Of course I had the bad luck opening the PR just after some dependencies updates break the build 😆

[smnandre]

Gave my reasons to not silent per default (we can change the receipe if you want !)

And i'm a bit surprised @Kocal tbh ...my memory is maybe incorrect, but i remember you pushing hard to trigger throwable in prod for AssetMapper ... 🤔

Anyway, to improve DX here, another thing could be to add some class or default icon when one is not found ?

[smnandre]

(Let's keep the current behaviour to prevent BC break, and improve DX with some catch/throw indeed when the flag is on, and set this flag default in next major? )

[Kocal]

And i'm a bit surprised @Kocal tbh ...my memory is maybe incorrect, but i remember you pushing hard to trigger throwable in prod for AssetMapper ... 🤔

Are you referring to symfony/recipes#1347 maybe? 😛 tl;dr: when developing, the AssetMapper was not able to resolve an asset. I didn't see a warning message was logged, I expected an exception to be thrown. However in production, a warning message instead of an exception is the perfect behavior (like twig.strict_variables etc).

To resume the PR, I think we all want:

• ux_icons.ignore_not_found to handle the "no internet connection" issue
• add a Symfony Recipe (cc Add a recipe for ux_icons, set default size #3172), something like:

ux_icons:
    # do not silence exceptions during rendering if icon is not found
    ignore_not_found: false

when@prod:
    ux_icons:
        ignore_not_found: true

Note that we can't directly use %kernel.debug% since we want to enable the option in production (where %kernel.debug% is false).

[Kocal]

Opened symfony/recipes#1477

[smnandre]

You defined the width/height at the same time 🤝

[Kocal]

The recipe has been merged