[Icons] Add ignore_not_found config option
#2023
Conversation
carsonbot
added
Feature
carsonbot
added
Status: Needs Work
| @@ -58,6 +58,10 @@ public function getConfigTreeBuilder(): TreeBuilder | |||
| ->end() | |||
| ->end() | |||
| ->end() | |||
| ->booleanNode('ignore_not_found') | |||
| ->info('Ignore error when an icon is not found.') | |||
| ->defaultFalse() | |||
There was a problem hiding this comment.
It could default to %kernel.debug% here or in the recipe
There was a problem hiding this comment.
I think the idea in #2008 was to default to "not kernel.debug".
I this change anything in prod, in would be a BC break, so maybe the recipe is a good solution, indeed
| try { | ||
| return $this->iconRenderer->renderIcon($name, $attributes); | ||
| } catch (IconNotFoundException $e) { | ||
| if ($this->ignoreNotFound) { |
There was a problem hiding this comment.
Because it's a Twig rendering context, I'm wondering if we should use Environment::isStrictVariables() instead. I can't picture a case where the twig.strict_variables config is different from ux_icons.ignoreNotFound. They serve the same purpose for me.
There was a problem hiding this comment.
I'm thinking this also, at least until we're asked for a separate config option.
There was a problem hiding this comment.
If by "using Environment::isStrictVariables()" you mean "do not throw if strictVariable = false", this would be a BC break, no ?
There was a problem hiding this comment.
Not throwing the exception in the prod env doesn't seem like a critical behavior change to me. In fact, that's exactly what the reporter is requesting (aligned with how Twig behaves). As suggested #2023 (comment) we could log the error instead.
The configs (strict_variables and ignore_not_found) appear to be opposite due to their naming, but they actually represent the same concept/purpose.
carsonbot
added
Status: Needs Review
|
Update: add a logger / log a warning when icon is not found |
|
-- I've been -more than once- told we cannot do this or that for way, way smaller BC breaks. But if you guys say it's ok, that's ok for me. --
Every other filter / function i know of throw in Twig when an error occurs The only silent thing is for getAttribute. To me an icon (which should be in cache) is nothing like an attribute on an (existing) object. The asset function throw when an asset does not exist (example: a svg icon) -- If we use strict_variables here, I cannot have exception in prod for missing icons anymore. (i would need to set strict_variables to true -- something i don't desire for obvious reasons) -- Not in the mood for the subtext of this discussion.. so just tell me your decision and i'll update the PR. |
kbond
force-pushed
the
feat/ignore-not-found
branch
from
8109a58 to
2dea170
Compare
|
This looks fine to me as is! |
Allow to silence error during rendering when an icon is not found (then a warning is log)