-
-
Notifications
You must be signed in to change notification settings - Fork 364
[Icons] Improve cache warmup by looking at every string #1594
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,11 +38,7 @@ public function icons(): array | |
foreach ($this->files($this->twig->getLoader()) as $file) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we exclude the Bundle ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What bundle? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point, i think Twig loader has been registered with all the bundles of the app. So it contains all the files in "vendor/acme/*-bundle/Resources/templates" and we should (imho) not go there to extract things. Imagine a twig file there with a list of icons (think any template using JS icons)... the probability to have 'bi:check' string is far from null. We even could see this as a vector "attack", a single file in twig from a 3rd party bundle would make you downlaod icons locally / generate a lot of download. Oh and i'll check something about that we may have a problem there. So here i'd like us to only look in userland files :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you had a bundle of that uses icons that you wanted to use? I prefer to keep it simple and adjust when/if we come across these type of problems. |
||
$contents = file_get_contents($file); | ||
|
||
if (preg_match_all('#ux_icon\(["\']([\w:-]+)["\']#', $contents, $matches)) { | ||
$found[] = $matches[1]; | ||
} | ||
|
||
if (preg_match_all('#name=["\']([\w:-]+)["\']#', $contents, $matches)) { | ||
if (preg_match_all('#["\']([\w:-]+)["\']#', $contents, $matches)) { | ||
$found[] = $matches[1]; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
{% set icon = 'flag:eu-4x3' %} | ||
|
||
{{ ux_icon('something:invalid') }} | ||
{{ ux_icon('invalid') }} | ||
{{ ux_icon('iconamoon:invalid') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If (i insist on if) this become the bottleneck, we have a card in hand: grouping icons per set https://iconify.design/docs/api/icon-data.html#query