No Code Attached Yet bug
avatar brianteeman
brianteeman
15 Feb 2023

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

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

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

avatar brianteeman brianteeman - open - 15 Feb 2023
avatar joomla-cms-bot joomla-cms-bot - change - 15 Feb 2023
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 15 Feb 2023
avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

There's a PR: #39866

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

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

avatar brianteeman
brianteeman - comment - 15 Feb 2023

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

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

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

avatar brianteeman
brianteeman - comment - 15 Feb 2023

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

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

you just don't understand

Ok, could you elaborate

avatar brianteeman
brianteeman - comment - 15 Feb 2023

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

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

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

avatar wojtekxtx
wojtekxtx - comment - 15 Feb 2023

Seriously @brianteeman: I dont get whats your point here (as well as in the other, closed, issue)? Oh, and @dgrammatiko is not trying to go on rant here, both his wording and his thinking process are correct here.

The core problem here can be that you look at this from different angle than @dgrammatiko or me. And this may be valid reason of confusion.

So we both (@dgrammatiko and I) asked you for clarification. As a reply you closed original issue and opened this one. For the sake of what?

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

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

avatar brianteeman
brianteeman - comment - 15 Feb 2023

finally ;)

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

@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

avatar wojtekxtx
wojtekxtx - comment - 15 Feb 2023

@dgrammatiko if Joomla cannot handle extension, than why not add support to it? Instead of opening multiple issues as well as fruitless comments exchange, maybe its time to add support for ico files to Joomla?

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

avatar brianteeman
brianteeman - comment - 15 Feb 2023

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

no. it is not related to the reported problem

avatar dgrammatiko
dgrammatiko - comment - 15 Feb 2023

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)

avatar wojtekxtx
wojtekxtx - comment - 15 Feb 2023

@brianteeman related or not, does not matter. You opened this double issue mess, so I want you to explain.

avatar joomdonation
joomdonation - comment - 17 Feb 2023

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

avatar dgrammatiko
dgrammatiko - comment - 17 Feb 2023

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

avatar joomdonation
joomdonation - comment - 19 Feb 2023

@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';
}
avatar Hackwar Hackwar - change - 22 Feb 2023
Labels Added: bug
avatar Hackwar Hackwar - labeled - 22 Feb 2023
avatar Hackwar Hackwar - change - 25 Aug 2023
Labels Added: PBF
avatar Hackwar Hackwar - labeled - 25 Aug 2023
avatar brianteeman
brianteeman - comment - 1 Sep 2023

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

avatar brianteeman brianteeman - change - 1 Sep 2023
Labels Removed: PBF
avatar brianteeman brianteeman - unlabeled - 1 Sep 2023
avatar brianteeman
brianteeman - comment - 3 Dec 2023

closing

avatar brianteeman brianteeman - change - 3 Dec 2023
Status New Closed
Closed_Date 0000-00-00 00:00:00 2023-12-03 11:40:00
Closed_By brianteeman
avatar brianteeman brianteeman - close - 3 Dec 2023

Add a Comment

Login with GitHub to post a comment