[#31410] - [4.0] htmlhelper svg
- Closed
- 28 Nov 2020
- Medium
- Build: 4.0-dev
- # 31410
- Diff
- N6REJ:4.0-htmlhelper-svg
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
changes when an error is checked for.
Testing Instructions
go to /administrator/index.php via https://
check for webauthn icon.
apply pr.
check that webauthn icon shows.
change /plugins/system/webauthn/src/PluginTraits/AdditionalLoginButtons.php
line 156 to 'svg' => HTMLHelper::_('icons.svg', 'http://hallhome.us/plg_system_webauthn/webauthn.svg', true),
check that icon is missing and no error shown.
change /plugins/system/webauthn/src/PluginTraits/AdditionalLoginButtons.php
line 156 to 'svg' => HTMLHelper::_('icons.svg', 'plg_system_webauthn/webauthn2.svg', true),
check that icon is missing and no error shown.
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required
This helper allows for svg's to be used as inline-svg's. This can be done by calling the helper class with the path to the image.
the webauthn plugin is a good example of how it can be used in place of the standard icon or image simply by replacing icon/image with 'svg', as shown here....
'svg' => HTMLHelper::_('icons.svg', 'plg_system_webauthn/webauthn.svg', true),
this would get the image from the /media/plg_system_webauthn/images folder.
if it was to false then it would get it from the exact path given.
Like all helpers it can be called directly in the code ( like a template for example ) if desired.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- 
| Category | ⇒ | Libraries |
| Title |
|
||||||
| Labels |
Added:
?
|
||
I have tested this item
Test instructions could be clearer.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31410.
I have tested this item
Test instructions could be clearer.
+1
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31410.
set RTC
afaik @dgrammatiko and @HLeithner oppose the way this new API is coded.
Until we have some precise feedback from them, I guess it is urgent to wait.
@sandewt my comment #31098 (comment) is still valid
this can be done by simply copying the svg files in the media/plg_vote_/images
or inlining the svg in the css as @infograf768 already did in some other part (#30516 )
Manually means inject the svg without the use of any API, eg echo '<svg>....</svg>'; but fwiw the css approach might be better there, less code
@dgrammatiko
I was not expecting a total reject of this new API but rather an improvement, if necessary.
I still don’ understand what is wrong with it btw.
I still don’ understand what is wrong with it btw.
Very briefly the most important parts (that can't be salvaged unless totally redesigned):
- Try this code:
<ul>
<?php for ($I=0; $I<11; $I++) { ?>
<li><?php echo HTMLHelper::_('icons.svg', 'some-svg.svg', true); ?></li>
<?php } ?>
</ul>where some-svg.svg is:
<svg height="599pt" viewBox="0 0 874 599" width="874pt" xmlns="http://www.w3.org/2000/svg">
<g transform="matrix(-1 0 0 1 874 0)">
<path id="randomId-1" d="m565.57719 153.84547c-57.44079 18.90106-108.60099 11.28936-154.72919 11.90653-21.248-4.072-43.112-5.496-64.736-7.44-30.992 2.4-60.616 15.416-85.936 32.96-43.264 12.888-86.384 34.16-132.888 30.496-41.776 4.416-79.0384-20.904-120.008-24.624-2.72032.096-6.81178823 6.35103-3.9932276 7.87939 21.1481046 11.46751 37.5894216 14.72389 45.9484916 19.10472 35.756 18.24 94.362276 21.89774 133.200736 19.27989 17.256-.576 33.952-4.192 49.76-10.248 7.496-3.608 11.848 6.96 10.656 13.048 13.04 50.168 27.76 99.936 40.776 150.112 8.04 32.144.416 66.112-16.584 93.96-3.44 10.536 5.84 19.008 10 27.68 10.152 23.192 35.936 42.968 28.76 70.848-3.904 10.848 25.192 6.176 7.872 4.6 4.728.088 14.968-5.904 8.456.968 8.664-1.496 15.904 4.816 24.32 3.504 1.632-4.76-11.248-4.408-4.656-10.648 4.92 8.424 4.32-2.56-1.864-4.928 6.104-1.488 7.76 6.896 11.024 10.536-3.56 1.16-2.464 3.672-.776 6.16h4.392c6.864-2.632.536-10.536-.368-15.048-4.744-14.64-26.144-10.952-29.488-26.76-12.184-27.192-27.128-63.24-4.224-88.712 13.944-17.08 33.4-29.568 42.752-50.176 19.368-24.928 21.928-58.28 18.352-88.432 13.65647-9.35801 18.1311-3.0599 33.84176-3.072 49.976 6.01503 80.01928 37.77833 121.97424 43.472s61.544 2.576 92.368 3.328c9.17985 11.38074 16.95713 94.02507 18.344 125.24 1.104 10.32 4.296 24.952 14.456 28.632 8.824-6.68 7.952 4.336 1.472 7.76 5.944 10.696 7.64 23.28 14.384 33.424 7.44 7.472 18.544 4.152 25.344-1.952 1.432-5.192-6.128-7.36-4.576-12.464 8.08-.52 14.736 6.224 15.648 14-2.368-3.496-4.832-5.824-5.936-.232 5.176 3.984 13.144 2.184 18.52-.856 1.096-14.624-12.728-24.904-26.536-24.776-11.056-14.528-17.696-33.688-15.048-52.072-2.68-34.784 4.472-69.312 5.344-104.08 3.96-26.072 8.48-52.936 22.088-75.912 21.4-27.152 15.08-63.768 21.232-95.688 2.61275-14.25838 5.2266-20.9566 12.3757-33.30224 10.264-22.1563 13.6761-37.44136 24.2521-52.96136 14.072-13.024 60.39299 5.49736 69.42616-20.63513 8.72-6.688-31.28996 2.30673-27.44996-7.32527 8.08-10.472 27.37366-1.897565 36.81366-4.393565 10.03473-5.493267 12.70292-.167118 16.38634-13.478435 0 0 2.53922-8.515135 2.85922-14.344735 0 0 4.82078-3.998737 4.82078-12.733665-1.52-4.7392-7.36-5.04-11.28-6.24-18.08-3.12-34.72-10.8992-52.48-15.2896-10.416-1.64-5.44-15.4896-13.36-19.9296-22.78603-11.4049658-37.20994-12.9336923-61.528-13.9904-21.35815 2.8352671-38.864 4.13-51.44 18.9504-17.37893 22.3496-37.17737 42.245898-43.87312 67.920087-12.44192 23.719473-29.69081 51.847273-74.46169 66.974983z"/><path d="m225.84964 286.45464c-3.25888 36.384-15.21764 63.50536-27.48964 96.96936-5.12 27.176-18.432 52.344-36.472 72.728-.024 28.296 1.624 55.736-7.544 83.256-3.856 9.032-3.272 30.512 12.008 22.168 7.248-9.488 13.52 4.296 21.768.88 6.968-1.264-1.168-10.632 7.832-6.584-6.36 3.912 1.288 10.016 4.992 4.624 1.048-13.6-12.104-21.744-15.84-33.784-2.208-20.264 3.536-41.92 17.264-57.352.36-14.448 7.872-28.808 20.12-36.864 18.52-15.776 43.93468-32.72445 57.12668-53.06045-11.936-44.68-25.10449-101.00831-37.29649-145.64031-6.64464 17.49993-5.46563 18.70493-16.46855 52.6594z"/><path d="m619.392 376.104c-1.952.032-4.168.4-6.68 1.168-5.464.608-3.48 7.96-2.24 11.208 7.248 33.088 17.656 66.656 16.872 100.224.16 6.184 7.056 6.024 10.424 9.768 9.904 1.368-3.088 1.24-2.208 5.888.88 11.672-.808 26.832 11.784 32.584 3.648-.744 15.504-.856 11.68-6.872-11.872-8.152-11.872-24.04-13.168-36.912-3.488-27.472-4.528-56.176-10.328-82.616-1.528-12.632-.216-34.696-16.136-34.44z"/></g></svg>So, there 2 major problems with this naive API:
- The svgs are echoed as they are meaning the API is inflating, naively the document, but worse than that
- it could produce non valid HTML (see that
id="randomId-1", this part needs to be unique)
If the project wants to go into the svg era, at least as I already proposed to @N6REJ, use my research and code from the 2 previous attempts...
none of the fontawesome svgs use id's so I don't see the relevance.
If I understand well, as the API is not reserved to Fontawesome svgs, looks like file_get_contents could include unwanted internal svg code.
If the solution requires a specific filtering of the svg concerned, remains to decide what has to be filtered to accept or not the svg.
@dgrammatiko Is that correct?
@dgrammatiko Is that correct?
Not really, it’s not about making the api fontawesome specific ( it’s name is svg not iconFa or something that dictates that this is only fa specific). So reserving a word for something that is not actually doing what is expected (eg inline any svg) is wrong.
But again the problems are deeper, the bloated content is like you can call 5 times HTMLHelper::(‘behavior.bootstrap’); and instead of getting one script tag you’ll get 5! This is why I call this thing naive!
Please revert it, inclining svgs need another approach, it’s already showcased in other prs
@dgrammatiko Is that correct?
Is it ? Asking as you just quoted my question.
EDIT: thank you, you now replied.
@HLeithner Your call now.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-11-28 10:23:27 |
| Closed_By | ⇒ | infograf768 |
@N6REJ the API you introduced in the previous PR is incomplete in so many aspects, the file check is the tip of the iceberg...