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

Template Manager ico #39861

Closed
brianteeman opened this issue Feb 14, 2023 · 21 comments
Closed

Template Manager ico #39861

brianteeman opened this issue Feb 14, 2023 · 21 comments

Comments

@brianteeman
Copy link
Contributor

Steps to reproduce the issue

Using the template manager try to upload an *.ico image such as a favicon
It should fail
Go to the template manager options and add ico to the list of alloed image formats

image

Try again to upload an *.ico image using the template manager

Expected result

File uploaded

Actual result

File Uploaded success message
AND
File not uploaded error message

image

System information (as much as possible)

Additional comments

@wilsonge
Copy link
Contributor

You need to add image/vnd.microsoft.icon to the Legal MIME Types field

We validate the file format and the mime type of the file as part of the security checks.

@brianteeman
Copy link
Contributor Author

This is in the template manager not the media manager where there is no mime type field and if twasnt clear the ico image is uploaded despite the error messages

image

@wilsonge
Copy link
Contributor

🤦 i'm blind. sorry.

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 14, 2023

.ico images are not valid for the img tag (ie cannot display inside an HTML doc): https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#supported_image_formats

Also you shouldn't use .ico for favicons anymore, use PNG: https://caniuse.com/link-icon-png

@brianteeman
Copy link
Contributor Author

@dgrammatiko that isnt the point. the conflicting messages is the issue

@dgrammatiko
Copy link
Contributor

The green alert is correct because the file was uploaded correctly

The error comes from

throw new \InvalidArgumentException('Attempting to load an image of unsupported type ' . $properties->mime);
which is coming from
$JImage = new Image(Path::clean($path . $fileName));

You could try to patch things, probably here (?):

Again, the ico files are not images for the browsers (or most web tools).

@brianteeman
Copy link
Contributor Author

not interested in debating the value of ico or if they are images or if they should be used.

This issue is purely about the two conflicting messages

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Feb 14, 2023

This issue is purely about the two conflicting messages

This is not an issue. ICO ARE NOT IMAGES (in the sense that jpg, png,avif,webp, gif) both for the browser and the PHP tools (the image class). Adding that extension to the allowed ones and expecting that somehow both PHP and Browsers will actually do something more than an error is delusional...

@brianteeman in case I wasn’t clear: you added a non valid image format into the array of valid images and expected somehow to work. Similarly you could add psd or ai and get the same error. If you need to add the ico format support you have to do it as an unknown file (like zip)

@brianteeman
Copy link
Contributor Author

that is NOT the issue I am reporting.

You should never be able to get both a success and a fail message for the same thing

@dgrammatiko
Copy link
Contributor

You should never be able to get both a success and a fail message for the same thing

Once again: you added a non valid image format into the array of valid images and expected somehow to work, that would never work because neither browsers or PHP GD is not considering .ico files as valid images.

@dgrammatiko
Copy link
Contributor

han this img type (ico in this case) should be made valid by adding support for it. Pretty obvious.

No. The web is trying to forget this weird format, use PNG or SVG is the best practice for some time now. Of course if you want to support it you could, ie in the browser use canvas to decode that thing and then write your own decoder/encoder for the PHP (since GD and Imagic don't support it).

@brianteeman
Copy link
Contributor Author

closing this and will re-open a new issue with a big warning to NOT discuss if an ico is an image or not

@Orgoth
Copy link

Orgoth commented Oct 5, 2023

You should never be able to get both a success and a fail message for the same thing

Once again: you added a non valid image format into the array of valid images and expected somehow to work, that would never work because neither browsers or PHP GD is not considering .ico files as valid images.

@dgrammatiko You are writing nonsense!

imagemagick is supporting ico "convert -list format | grep ICO -> ICO* ICON rw+ Microsoft icon"
tested under Kubuntu 22.04 LTS and Debian 12 Server "ImageMagick 6.9.11-60 Q16 aarch64 2021-01-25"
Even the current release 7.1 does support ico.
https://github.com/ImageMagick/ImageMagick

Nearly all Browsers are supporting ico as icon-image format.
https://en.wikipedia.org/wiki/Favicon#Browser_implementation
https://en.wikipedia.org/wiki/Favicon#HTML5_recommendation_for_icons_in_multiple_sizes

HTML5 recommendation for icons in multiple sizes
The current HTML5 specification recommends specifying multiple sizes for the icons, using the attributes rel="icon" sizes="space-separated list of icon dimensions" within a tag.[39] Multiple icon formats, including container formats such as Microsoft .ico and Macintosh .icns files, as well as Scalable Vector Graphics may be provided by including the icon's content type in the format type="file content-type" within the tag.

Only GD-Lib does not support the ico format.

.ico images are not valid for the img tag (ie cannot display inside an HTML doc): https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#supported_image_formats

Have a look at the link tag.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#examples:~:text=There%20are%20a%20number%20of%20other%20common%20types%20you%27ll%20come%20across.%20For%20example%2C%20a%20link%20to%20the%20site%27s%20favicon%3A

<link rel="icon" href="favicon.ico" />

A lot of pagebuilders are using the image folder to select the favicon and all Browsers are still supporting ico as favicon.

Since you have referred to caniuse, I will also do so.
https://caniuse.com/mdn-html_elements_link_rel_icon

Auswahl_525

will use ICO over PNG if ICO has better matching sizes set
safari: If both ICO and PNG are available, will ALWAYS use ICO file, regardless of sizes set.

Only the manifest icons does not support ico in all cases. "or is simply unknown if it is supported"
https://caniuse.com/mdn-html_manifest_icons

@dgrammatiko
Copy link
Contributor

Nearly all Browsers are supporting ico as icon-image format.

My comment was: .ico images are not valid for the img tag (ie cannot display inside an HTML doc): which is not about the <link rel=icon>!!!!
You don't even understand the problem but you thought you'll give some input (and calling my comments nonsense). So, kindergarten explanation:

  • Open your favourite browser
  • Drag and drop any image file
  • The supported formats will be rendered in the browser
  • Guess what? The .ico file WILL NOT, You know why? Because that image format is not supported by the browsers (for the HTML pages, only as an icon for their chrome tabs!

All your other comments are irrelevant

@Orgoth
Copy link

Orgoth commented Oct 5, 2023

Nearly all Browsers are supporting ico as icon-image format.

My comment was: .ico images are not valid for the img tag (ie cannot display inside an HTML doc): which is not about the <link rel=icon>!!!! You don't even understand the problem but you thought you'll give some input (and calling my comments nonsense). So, kindergarten explanation:

  • Open your favourite browser
  • Drag and drop any image file
  • The supported formats will be rendered in the browser
  • Guess what? The .ico file WILL NOT, You know why? Because that image format is not supported by the browsers (for the HTML pages, only as an icon for their chrome tabs!

All your other comments are irrelevant

I did and you know what, it is working. xD

Version 117.0.5938.132 (Offizieller Build) (64-Bit)
Auswahl_532

Firefox 118.0.1 (64-bit)
Auswahl_533

@Orgoth
Copy link

Orgoth commented Oct 5, 2023

Nearly all Browsers are supporting ico as icon-image format.

My comment was: .ico images are not valid for the img tag (ie cannot display inside an HTML doc): which is not about the <link rel=icon>!!!! You don't even understand the problem but you thought you'll give some input (and calling my comments nonsense). So, kindergarten explanation:

  • Open your favourite browser
  • Drag and drop any image file
  • The supported formats will be rendered in the browser
  • Guess what? The .ico file WILL NOT, You know why? Because that image format is not supported by the browsers (for the HTML pages, only as an icon for their chrome tabs!

All your other comments are irrelevant

And so that you can't claim that it only works because it was loaded directly via the file system, here again with an html file, with an img and the favicon.

Auswahl_534

Auswahl_535

<!DOCTYPE html>
<html lang="">
  <head>
    <meta charset="utf-8">
    <link rel="icon" href="mrmo_favicon_test.ico" />
    <title></title>
  </head>
  <body>
    <header></header>
    <main><img src="mrmo_favicon_test.ico"></main>
    <footer></footer>
  </body>
</html>

@Orgoth
Copy link

Orgoth commented Oct 5, 2023

And vivaldi 6.2.3105.54 (Stable channel) stable (64-Bit) :

Auswahl_536

Edge Version 117.0.2045.55 (Offizielles Build) (64-Bit)

ae5hae5hueah5traertae5zuhe5uj

Opera GX LVL5 (core: 102.0.4880.90)

jrts6risr6ir6isr6irzkjftuk

@dgrammatiko
Copy link
Contributor

@Orgoth probably this is new but personally I'll stick to the SPECS: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#supported_image_formats and not do anything with that legacy proprietary format, the web has moved on...

@Orgoth
Copy link

Orgoth commented Oct 5, 2023

That is also absolutely right that it must continue or move on.
Nevertheless, it is a format that is supported by all common browsers, be it for legacy reasons or not. "and imagemagick"
Accordingly, the statements you made were unfortunately wrong and could not stand.

As communicated by me, some page builders, such as Yootheme use the image's folder for the favicons and the ico cannot be uploaded even if the user enables them.
But this is a separate point that does not belong here to the actual ticket, because this is just a duplicate message, as you have also come to the same conclusion.

#39865 (comment)

@Orgoth probably this is new but personally I'll stick to the SPECS: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#supported_image_formats and not do anything with that legacy proprietary format, the web has moved on...

Even in this case, you are talking nonsense.

Quote from the Spec you are referring to:

Supported image formats
The HTML standard doesn't list what image formats to support, so user agents may support different formats.

Note: The Image file type and format guide provides comprehensive information about image formats and their web browser support. This section is just a summary!

The image file formats that are most commonly used on the web are:

@dgrammatiko
Copy link
Contributor

If you are using tools that are still using .ico files I guess you could use better your time to fill issues to their support so they could move to something more modern (png or even better svg) instead of fighting over a closed issue...

Unsubscribing

@Orgoth
Copy link

Orgoth commented Oct 5, 2023

@dgrammatiko
It was solely your manner, as well as the false factual claims of you, that prompted me to write anything at all about this.
Also, that you have not answered questions that were addressed to you directly.
See @wojtekLs
And that you just ignored the obvious problem, what @brianteeman reported and permanently stuck to the point of view that the format is weird and outdated or not recognized as an image.
#39861 (comment)
The image is clear, there is a success message and an exception and there should be only one of them.

Please do not make false claims without having practically counter-checked that this is indeed the case.

Yes lets close this discussion here, we have made our point clear.

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

No branches or pull requests

5 participants