[Icons] Add xmlns attribute to svg icons

[Enz000]

Q	A
Bug fix?	no
New feature?	yes
Docs?	no
Issues	Fix #2561
License	MIT

The xmlns attribute was not present in the downloaded SVG icons.
To follow the W3C standard, I added it inside the svg tag.

[smnandre]

Problem is you now add it for everyone, everywhere, even in the inline rendering, where this is not required (and has no realy meaning).

I do believe we should try to implement custom attribute allowing/disallow list

[Kocal]

Problem is you now add it for everyone, everywhere, even in the inline rendering, where this is not required (and has no really meaning).

Is it really an issue? Sure it may be not useful when inlining, but is it breaking the document or the browser?

Popular icons "libraries" like HeroIcons or FontAwesome or Google's Icons provides SVG icons with a xmlns attribute, and no one tells you to remove it if you want to inline the svg in your document.

Let's be pragmatic, always adding this xmlns attribute is fine

[norkunas]

Let's be pragmatic, always adding this xmlns attribute is fine

which will break many tests for us

[Kocal]

Let's be pragmatic, always adding this xmlns attribute is fine

which will break many tests for us

Even if the rendered HTML changes, as long as it doesn't break rendering in the browser, that's totally fine.

We understand your tests will break, but we do not really cares here, it's not like we are removing some HTML attributes or whatever more impactful.

Also, note that we already did this kind of "breaking changes" on ComponentAttributes rendering because of performance issues, and we already stated that these kind of changes were fine.

Thanks for your comprehension!

[norkunas]

Sorry,but this will expand html size for no reason for us ... Im not against it, but at least config option would be ok

[Enz000]

Sorry,but this will expand html size for no reason for us ... Im not against it, but at least config option would be ok

The HTML size is not a valid reason for me. The attribute’s length is about 44 characters...

[norkunas]

and when you rendering 100 icons in same page it becomes 440 :)

[Kocal]

I'm pretty sure you webserver can compress your HTML response...

[Enz000]

and when you rendering 100 icons in same page it becomes 440 :)

True, but 440 characters represents almost 0,43 ko !
Still nothing but maybe I'm mistaken

[norkunas]

i still stand with @smnandre . do as you wish, i will write a script to clean the files

[norkunas]

I'm pretty sure you webserver can compress your HTML response...

but the repository size also grows

[Kocal]

Please let's keep the discussion pertinent, I don't think the arguments "my tests will break" nor the "HTML or repo size will grow" are resonable enough to prevent this feature to ship. Otherwise, shouldn't we stop developing things and remove code only? 😬

To move forwards @Enz000, you still need to update Icons tests and we will be fine. Maybe add one or two lines in the change log about the "breaking" thing.

Thanks everyone!

[norkunas]

As I said I'm not against it, just do not understand why such a rare need is pushed without opt-in for all users

[smnandre]

I'm sure we can all be a bit more chill here (and respectful).

I think all points are valid here:

UX Icons was made to render SVG icons in Twig. So I do agree this use case does not feel a valid reason to change the HTML generated by everyone else.

On the other side, not having this attribute can be a real pain (at least during development) raising a valid DX problem here. And making it hard to use the icons for anything else than inline rendering. So I do agree this use case does represent a valid reason to change something.

Ideally, this situation would have require just a tiny bit of goodwill, some people giving a bit of energy or time (like really not much here) to implement something more clever/adapted.

I already mentioned on another issue (quesstion from Javier i think) the following idea: we could add a include/exclude system of attributes, globally and/or by set and/or by usage (in file / at render time), as this seems to be very opiniated debates.

So to me forcing this attribute for everyone and storing it in cache is not the best idea, clearly. I suppose this would maybe be something to be done in the commands, via an Input Option.

But I don't want either to "penalize" someone that took some time to explain its problem, open a PR to try something, etc.

Have a good week-end you guys

[smnandre]

LGTM!

[Kocal]

It must be added in a new section for 2.24:

Suggested change

	- Add `xmlns` attribute in downloaded svg.
	## 2.24.0
	- Add `xmlns` attribute in downloaded svg.

[Kocal]

Also, as discussed in the PR but also with @smnandre yesterday, about the ""breaking changes"", it is true it can happens when you didn't lock your icons, and your tests/CI always fetch and re-fetch icons for Iconify.

Maybe we can add few words about the impacts and how to deal with, something like this:

Suggested change

	- Add `xmlns` attribute in downloaded svg.
	## 2.24.0
	- **BREAKING** Add `xmlns` attribute to icons downloaded with Iconify, to allows icons to be correctly rendered in browser as an external file, in SVG editors, and in files explorers or text editors previews.
	This is **an accepted breaking change** since **we did not modified the current rendering behavior in the browser** and it won't break your website. However it **may breaks your pipeline** if your tests assert on `ux_icon()` or `<twig:ux:icon>` output and you [didn't lock your icons](https://symfony.com/bundles/ux-icons/current/index.html#locking-on-demand-icons).
	We recommend you to **lock** your icons **before** upgrading to UX Icons 2.24. We also suggest you to to **force-lock** your icons **after** upgrading to UX Icons 2.24, to add the attribute `xmlns` to your icons already downloaded from Iconify.

[Kocal]

cc @norkunas

[norkunas]

Actually I always import them, to avoid useless http requests :) so at least all previously imported icons will be clean

[smnandre]

As I see it, we have two options to see it:

• If it’s a breaking change, we don’t release it.
• If it’s not a breaking change, there’s no need to make a big deal out of it.

But releasing it with BREAKING in the changelog does not feel a good idea.

So which one it is ? 😅

[chalasr]

Trying to cover so strictly the shape of the retval of a public method that isn't even documented code-wise is calling for trouble :) Putting that sentence in the PR body in case anyone want to read it should be more than enough

[norkunas]

If I will set ux_icon.default_icon_attributes.xmlns config option to false will it avoid rendering it at least? If no - would it be possible to make this work?

[smnandre]

I'd like we don't make a custom option per attribute, and use something more generic / once for all if possible.

But a temporary hard-coded option would be acceptable here, until someone has time to make clean things.

Discussion / implementation ideas here: #2353

[norkunas]

@smnandre I didn't said about custom option, I am exploring from what's available: default_icon_attributes