[Icons] hint about the http client when icon not found #2667
Conversation
|
test failures seem unrelated |
| @@ -38,6 +39,11 @@ public function get(string $name): Icon | |||
| } | |||
| } | |||
|
|
|||
| throw new IconNotFoundException(\sprintf('Icon "%s" not found.', $name)); | |||
| $exceptionExtra = ''; | |||
There was a problem hiding this comment.
The ChainIconRegistry has no reason here to be aware of this or that specific implementation.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
I like this idea very much too!
the neat part about that would be that other registries could also start adding hints
yep!
|
closing in favor of #2678 |
make exception more helpful when icon is not found.
this is a bit ugly because the iconfiy registry is not added to the chain if the http client is not available, so we can't know what exactly the user intention was.
i added a check for
:in the name to at least only trigger this when it could be an iconfiy icon.