Template Manager ico validation

[brianteeman]

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 allowed image formats

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

This is not about if an ico is an image or if it should be used or any other off topic issue. This is purely about getting two contradictory messages. Any off topic post I will request to be removed by the JBS

[dgrammatiko]

There's a PR: #39866

[dgrammatiko]

@brianteeman the problem is that you're adding a proprietary image format that is not supported by either the HTML specs or the PHP GD library and somehow you expect to somehow magically work. Joomla is just leveraging the existing formats, either in the PHP or in the HTML world, so you can't somehow magically add the .psd format (which is also proprietary) and expect that both PHP and HTML will do something meaningful.

In short supporting the ico is possible (I just created a PR) but adding the .ico files as supported images is a totally different case and for the HTML specs won't ever happen (due to proprietary nature). Same goes for the PHP which is just adding the formats that the HTML spec support.

[brianteeman]

you just cant help yourself from completely ignoring the reported issue and going on a rant.

[dgrammatiko]

It’s not a rand. What you are reporting is the same thing as me adding a psd file format in the allowed images and expect Joomla to somehow do something meaningful. In the previous issue I had a link to the HTML specs were they state the supported formats and these are the only ones allowed in that field! So basically your issue is a misunderstanding of what is allowed/valid image format for the specific field

[brianteeman]

you just dont understand - never mind - you will eventually - and then you will realise that you are not talking about the same thing as me

[dgrammatiko]

you just don't understand

Ok, could you elaborate

[brianteeman]

if you try to do something you should get a success or a fail.
you should not get both

[dgrammatiko]

you should not get both

The root problem here (and also in the Media Manager) is that these kind of fields shouldn't allow the typing the formats/extensions, etc rather they should had a pool of possible values and let the user pick the ones they want. This had been raised before: #34634 (comment)

In short both the template manager and the media manager should not allow users to pass non valid extensions, formats in the respected fields because that would cause such inconsistent behaviour

[dgrammatiko]

@wojtekxtx actually Brian is right here, the field should not accept any extension that Joomla could not handle. I was confused because I thought he was asking for support for the ico extension in the allowed/valid image extensions. The key part is the It should fail that I missed in both issues!

[brianteeman]

finally ;)

[dgrammatiko]

@brianteeman using this pattern, on the field the front end part could fail on invalid extensions. Of course this needs a field validation also in the PHP

^[gif|jpg|jpeg|png|webp|avif]+(,[gif|jpg|jpeg|png|webp|avif]+)*$

Live: https://regex101.com/r/hbC1pf/1

[brianteeman]

I still fail to see whats wrong in ico files. @brianteeman: dare to explain?

no. it is not related to the reported problem

[dgrammatiko]

maybe its time to add support for ico files to Joomla?

The web is moving away from the ico files for the favicon so personally I wouldn't like to see Joomla adding so late support for them (although I already open a PR for that)

[joomdonation]

@dgrammatiko Just looked at this issue. The reason of this error is because we are using Image class to detect size of the uploaded image https://github.com/joomla/joomla-cms/blob/4.2-dev/administrator/components/com_templates/src/Model/TemplateModel.php#L1461 but our Image class could not handle that ico image type.

One solution is that instead of using Image class like that, we can just use getimagesize function to get size instead.

$info = getimagesize(Path::clean($path . $fileName));

if ($info !== false)
{
	$image['height']  = $info[0];
	$image['width']   = $info[1];
}
else
{
	$app->enqueueMessage('Could not detect image size');
}

Just a bit of info from code reading. Don't ask me to make PR because I am not familiar with com_templates, so could easily miss something.

[dgrammatiko]

@joomdonation I had come to the same conclusion (Image class was breaking for ico) #39861 (comment)

FWIW I had a PR adding support for the .ico files #39866 though it needs some js, ie a canvas and a custom decoder for displaying the icon (stackoverflow?) but the issue Brian is reporting has to do with non existing validation of the allowed image extensions. You could add PSD, TIFF or whatever other image format and still have the same double messages...

[joomdonation]

@dgrammatiko I spent sometime to play with com_templates today and understand a bit more how it works. So for this issue, the error happens because after the file uploaded, we redirect user to image editing page to allow them to edit further but the image is not supported by our image API

How about after the file uploaded, if it is one of the image types which can be edited, we redirect them to image editing page? Otherwise, redirect them back to template files management page. So the code on this line https://github.com/joomla/joomla-cms/blob/4.2-dev/administrator/components/com_templates/src/Controller/TemplateController.php#L533 could be changed to:

if (in_array(File::getExt($return), ['gif', 'jpg', 'jpeg', 'png', 'webp', 'avif'])) {	
	$url = 'index.php?option=com_templates&view=template&id=' . $id . '&file=' . $redirect . '&isMedia=' . $this->input->getInt('isMedia', 0);
} else {
	$url = 'index.php?option=com_templates&view=template&id=' . $id;
}

Also, on this block of code https://github.com/joomla/joomla-cms/blob/4.2-dev/administrator/components/com_templates/src/View/Template/HtmlView.php#L189-L195 , we should also catch InvalidArgumentException so that if users attempt to edit one of the none supported image types, they will see a warning messages instead of seeing an error from uncatch Exception at the moment. Something like:

try {
	$this->image = $this->get('Image');
	$this->type  = 'image';
} catch (\RuntimeException $exception) {
	$app->enqueueMessage(Text::_('COM_TEMPLATES_GD_EXTENSION_NOT_AVAILABLE'));
	$this->type = 'home';
}
catch (\InvalidArgumentException $exception) {
	$app->enqueueMessage('Editing this image type is not supported');
	$this->type = 'home';
}

[brianteeman]

@joomdonation that explains the double (and conflicting messages) and looks like a sensible solution

[brianteeman]

closing