Skip to content
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

ux_icon triggers an exception when icon doesn't exists #2008

Closed
Seb33300 opened this issue Jul 25, 2024 · 5 comments · Fixed by #2023
Closed

ux_icon triggers an exception when icon doesn't exists #2008

Seb33300 opened this issue Jul 25, 2024 · 5 comments · Fixed by #2023

Comments

@Seb33300
Copy link

Seb33300 commented Jul 25, 2024

Description

The twig ux_icon function always triggers an exception when the icon name doesn't exists.

In my opinion, since icons are usually not a critical feature (most of the time only for design purpose), this should trigger an exception only in dev mode by default and the exception should be catched with an error message in logs in prod mode.

Maybe by adding an option to the UX Icons component to configure this?

Example

I have a feature in my app where the admin can add icons to display in the app.
One icon has been added with a typo and this put few pages of our site down.

@Seb33300 Seb33300 changed the title ux_icon trigger an exception when icon doesn't exists ux_icon triggers an exception when icon doesn't exists Jul 25, 2024
@xabbuh xabbuh transferred this issue from symfony/symfony Jul 26, 2024
@smnandre
Copy link
Member

Really sorry to hear that.

Unfortunately, many users expect an exception in this specific case, so i don't think we would silent them here.

However.... we recently merged #1889, which exposes an interface for the IconRenderer and will be available in the next release (in the coming weeks).

You could then decorate the IconRenderer to catch potential exceptions?

@smnandre smnandre added the Icons label Jul 26, 2024
@Kocal
Copy link
Member

Kocal commented Jul 27, 2024

Unfortunately, many users expect an exception in this specific case, so i don't think we would silent them here.

I don't really agree here, you don't want you app to crash if an icon is not able to be rendered. I understand the benefit in thrownig an error, but in development only.

Before Symfony UX Icon, we used things like '<i class="fa-solid fa-'~ iconName ~'"></i>' and it didn't crash the whole app.

Something like a strict mode, like Twig and variables. Depending of the configuration (by default %kernel.debug% if I remember well), it may throw an exception or not.

@smnandre
Copy link
Member

if an icon is not able to be rendered

That's very debatable :)

So yeah, a configuration option could please everyone.

And what about a "default" icon ?

@smnandre
Copy link
Member

smnandre commented Aug 1, 2024

Hi @Seb33300 : i tried something with a config option... in #2023 If by chance you had time to check and see if everything works for you that'd be very nice :)

@Seb33300
Copy link
Author

Seb33300 commented Aug 2, 2024

Hi @Seb33300 : i tried something with a config option... in #2023 If by chance you had time to check and see if everything works for you that'd be very nice :)

Thank you for that PR!
Just tested it and it seems to work.
My only concern is to add errors to logs so we can still have a way to find and fix them.

@kbond kbond closed this as completed in 1b4c6b3 Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants