Skip to content
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

[4.0] htmlhelper svg #31410

Closed
wants to merge 4 commits into from
Closed

[4.0] htmlhelper svg #31410

wants to merge 4 commits into from

Conversation

N6REJ
Copy link
Contributor

@N6REJ N6REJ commented Nov 15, 2020

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

image

Expected result AFTER applying this Pull Request

image

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.

@N6REJ N6REJ changed the title 4.0 htmlhelper svg [4.0] htmlhelper svg Nov 15, 2020
@dgrammatiko
Copy link
Contributor

@N6REJ the API you introduced in the previous PR is incomplete in so many aspects, the file check is the tip of the iceberg...

Co-authored-by: Brian Teeman <brian@teeman.net>
@N6REJ
Copy link
Contributor Author

N6REJ commented Nov 15, 2020

@N6REJ the API you introduced in the previous PR is incomplete in so many aspects, the file check is the tip of the iceberg...

You're welcome to create a pr to extend it.

@sandewt
Copy link
Contributor

sandewt commented Nov 24, 2020

I have tested this item ✅ successfully on bfbba71

Test instructions could be clearer.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31410.

@ghost
Copy link

ghost commented Nov 25, 2020

I have tested this item ✅ successfully on bfbba71

Test instructions could be clearer.

+1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31410.

@sandewt
Copy link
Contributor

sandewt commented Nov 25, 2020

Note: this PR is required for the proper functioning of:

[4.0] Improvement Vote Plugin #31098

@ghost
Copy link

ghost commented Nov 27, 2020

set RTC

@infograf768
Copy link
Member

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.

@dgrammatiko
Copy link
Contributor

Fwiw since this api is really wrong I will ask to revert it and insert manually in the svgs wherever this was used. Manually placing svgs was also @wilsonge decision until we figure out an api that satisfies all parties and constrains.

@sandewt
Copy link
Contributor

sandewt commented Nov 27, 2020

Fwiw since this api is really wrong I will ask to revert it and insert manually in the svgs

@dgrammatiko

@dgrammatiko
Copy link
Contributor

@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

@infograf768
Copy link
Member

@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.

@dgrammatiko
Copy link
Contributor

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...

@N6REJ
Copy link
Contributor Author

N6REJ commented Nov 28, 2020

none of the fontawesome svgs use id's so I don't see the relevance.

@infograf768
Copy link
Member

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
Copy link
Contributor

dgrammatiko commented Nov 28, 2020

@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

@infograf768
Copy link
Member

infograf768 commented Nov 28, 2020

@dgrammatiko Is that correct?

Is it ? Asking as you just quoted my question.

EDIT: thank you, you now replied.

@HLeithner Your call now.

@infograf768
Copy link
Member

@N6REJ @dgrammatiko @sandewt

Please see #31516
Maintainers decided to revert #31190 until a correct API is created in 4.1 or later.
I am now closing this one as it will never be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants