Skip to content

[Icons] hint about the http client when icon not found #2667

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

Closed
wants to merge 1 commit into from
Closed

[Icons] hint about the http client when icon not found #2667

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
8 changes: 7 additions & 1 deletion src/Icons/src/Registry/ChainIconRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\UX\Icons\Registry;

use Symfony\Component\HttpClient\HttpClient;
use Symfony\UX\Icons\Exception\IconNotFoundException;
use Symfony\UX\Icons\Icon;
use Symfony\UX\Icons\IconRegistryInterface;
Expand Down Expand Up @@ -38,6 +39,11 @@ public function get(string $name): Icon
}
}

throw new IconNotFoundException(\sprintf('Icon "%s" not found.', $name));
$exceptionExtra = '';
Copy link
Member

Choose a reason for hiding this comment

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

The ChainIconRegistry has no reason here to be aware of this or that specific implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree on the principle, but this is the exception everyone will see when they did not install the http client but want to use iconify. i think we really should have some way to alert people, and having this code here does nothing functional, it just provides a valuable hint.

i did not investigate if the iconify registry is used and fails, but would assume its not even loaded by the bundle. if it is loaded, we could introduce a special case of IconNotFoundException that provides hints, and have the chain registry collect hints and append them. that seems rather complicated but would be cleaner - except it would require adding not working registries to the chain, incurring an overhead always, also for users who actually don't use iconify.

do you have another idea how to do this? i think its quite important from a DX perspective that the exception explains the problem properly.

Copy link
Member

Choose a reason for hiding this comment

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

everyone will see when they did not install the http client but want to use iconify

This is where i think we could probably improve things. Either during kernel configuration, or maybe on the class really calling this code.

But sincerely i don't find a good idea to hard-code such detection here. And i do understand your case was annoying, and absolutely love the fact you come here to improve the DX for everyone, I really do.

I also think this scenario is not the most common for this package and would simply find a good balance between these safenets and the other use cases.

This is why i suggested we add the http-client-contracts as a dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at ux.symfony.com/icons, it looks to me like the ability to use those 3rd party icon sets is an important feature of this library. but i of course don't know how people actually use it.

i am very convinced however, that when i use it with missing dependencies, i want the exception to tell me what the problem is. i don't know how adding http-client-contracts would help here, and adding the http-client as hard dependency seems odd if you say the usage to use 3rd party icon sets is uncommon.

we could of course register a dummy registry if there is no http client available, to always throw a helpful exception, and have the chain registry pick up helpful exceptions to provide a list of hints from registries. that would certainly be the clean solution. if the dummy registry comes last, it would also not add overhead except for the failure case of the other registries not finding the icon, which i would expect to be rare. the neat part about that would be that other registries could also start adding hints, for whatever things people are doing with their custom registries.

Copy link
Member

Choose a reason for hiding this comment

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

we could of course register a dummy registry if there is no http client available, to always throw a helpful exception

This was my first thought too!

Copy link
Member

Choose a reason for hiding this comment

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

I like this idea very much too!

the neat part about that would be that other registries could also start adding hints

yep!

if (str_contains($name, ':') && !class_exists(HttpClient::class)) {
$exceptionExtra = ' If you want the on-demand registry to fetch the icon from iconfiy, you need to install "symfony/http-client".';
}

throw new IconNotFoundException(\sprintf('Icon "%s" not found.%s', $name, $exceptionExtra));
}
}
Loading