? Pending

User tests: Successful: Unsuccessful:

avatar ralain
ralain
26 Mar 2016

Summary of Changes

Currently when uploading a file through the Media Manager with a space in its name an error is thrown. All other unwanted chars are already removed by JFile::makeSafe() but this method explicitly allows spaces.

Errors tend to confuse inexperienced users, so this PR ensures that spaces are replaced with dashes instead of causing a failed upload.

Testing Instructions

  1. In the Media Manager attempt to upload a file with one or more spaces in its name. Upload should fail.
  2. Apply patch.
  3. Repeat step 1. Upload should succeed and spaces in the filename be replaced with dashes.
avatar ralain ralain - open - 26 Mar 2016
avatar ralain ralain - change - 26 Mar 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Mar 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 26 Mar 2016
Category Media Manager
avatar brianteeman brianteeman - test_item - 26 Mar 2016 - Tested successfully
avatar brianteeman
brianteeman - comment - 26 Mar 2016

I have tested this item :white_check_mark: successfully on a7709ed


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

avatar MATsxm MATsxm - test_item - 26 Mar 2016 - Tested successfully
avatar MATsxm
MATsxm - comment - 26 Mar 2016

I have tested this item :white_check_mark: successfully on a7709ed

Able to reproduce then #9608 works as described - Thanks


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

avatar brianteeman brianteeman - change - 26 Mar 2016
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 26 Mar 2016

Maybe we need to update the error message as well
COM_MEDIA_ERROR_WARNFILENAME="File name must only contain alphanumeric characters and no spaces."


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

avatar joomla-cms-bot joomla-cms-bot - change - 26 Mar 2016
Labels Added: ?
avatar ralain
ralain - comment - 26 Mar 2016

The error message is triggered by JHelperMedia::canUpload() so I think the message would still be accurate if someone used that method directly without sanitizing the filename first.

avatar JoomliC
JoomliC - comment - 27 Mar 2016

This reminds me this issue #7841 ( @brianteeman still open but could be closed as issue seems already fixed, even if i don't know when, and in which PR...) and my comment about spaces in file upload : #7841 (comment)

So i agree to accept space on upload :+1: , but i prefer dash - as replacement.

In Google’s search engine algorithm, the underscore isn’t a recognized word separator.

For example:
my-new-black-kitten.jpg will be seen as 4 (key)words my new black kitten
my_new_black_kitten.jpg will be seen as 1 word mynewblackkitten

Another link with simple and clear explanation : http://searchengineland.com/9-seo-quirks-you-should-be-aware-of-146465

So... dash or underscore ?

avatar ralain
ralain - comment - 27 Mar 2016

I actually prefer the dash myself but went with underscore as I (mistakenly) thought that was the more accepted word separator. Happy to change it.

avatar brianteeman
brianteeman - comment - 27 Mar 2016

I thought it was an underscore as well

avatar JoomliC
JoomliC - comment - 27 Mar 2016

After many search since months, underscore seems to be used when it's an icon, a design element... in fact when you don't want and need to list this image in search engines.
Unfortunately, nothing definitely stated as a norm.

As image is included in a url (<img src="url-to-image"/>, or <a href="url-to-image">, metadata img...),
i am personally for a url safe output...

But this would mean to replace this:
$file['name'] = JFile::makeSafe($file['name']);

By this (the first 3 lines. Last ones are what i use in my own extension for empty sanitized $filename (eg. Asian characters filename)):

$filename = JFilterOutput::stringURLSafe(JFile::stripExt($file['name']));
$ext = strtolower(JFile::getExt($file['name']));
$file['name'] = $filename . '.' . $ext;

// If filename empty (for example, if you have a filename like this 代替品.jpg, it is currently returning the same error as with spaces in current J version), filename based on current date/time
if ( ! $filename)
{
    $file['name'] = JFactory::getDate()->format("YmdHis") . '.' . $ext;
}

This will return a lowcase dash separated string, the same way alias are generated.
For example, Spread the Joomla! Love.JPG will be spread-the-joomla-love.jpg
(today, with this PR, it would be Spread_the_Joomla_Love.JPG, not the best url SEO-friendly filename...)

More discussion here (and not having a clear position from all...): #7841 (comment)

In fact, a decision from PLT and translators, would help to decide the good way to go!

avatar brianteeman
brianteeman - comment - 27 Mar 2016

Why are underscore not URL safe?
On 27 Mar 2016 2:26 pm, "Cyril Rezé" notifications@github.com wrote:

After many search since months, underscore seems to be used when it's an
icon, a design element... in fact when you don't want and need to list this
image in search engines.
Unfortunately, nothing definitely stated as a norm.

As image is included in a url, i am personally for a url safe output...
But this would mean to replace this:
$file['name'] = JFile::makeSafe($file['name']);

By this (the first 3 lines. Last ones are what i use in my own extension
for empty sanitized $filename (eg. Asian characters filename)):

$filename = JFilterOutput::stringURLSafe(JFile::stripExt($file['name']));
$ext = strtolower(JFile::getExt($file['name']));
$file['name'] = $filename . '.' . $ext;

// If filename empty (for example, if you have a filename like this 代替品.jpg, it is currently returning the same error as with spaces in current J version), filename based on current date/time
if ( ! $filename)
{
$file['name'] = JFactory::getDate()->format("YmdHis") . '.' . $ext;
}

This will return a lowcase dash separated string, the same way alias are
generated.
For example, Spread the Joomla! Love.JPG will be
spread-the-joomla-love.jpg
(today, with this PR, it would be Spread_the_Joomla_Love.JPG, not the
best url safe filename...)

More discussion here (and not having a clear position from all...): #7841
(comment)
#7841 (comment)

In fact, a decision from PLT and translators, would help to decide the
good way to go!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9608 (comment)

avatar JoomliC
JoomliC - comment - 27 Mar 2016

@brianteeman they are safe, but not SEO friendly.

avatar JoomliC
JoomliC - comment - 27 Mar 2016

@brianteeman direct link to video of Matt Cutts, in case you didn't watch it already in one of the previous links i posted ;-)
https://www.youtube.com/watch?v=AQcSFsQyct8

avatar brianteeman
brianteeman - comment - 27 Mar 2016

I was only querying this statement of yours

(today, with this PR, it would be Spread_the_Joomla_Love.JPG, not the
best url safe filename...)

On 27 March 2016 at 15:23, Cyril Rezé notifications@github.com wrote:

@brianteeman https://github.com/brianteeman direct link to video of
Matt Cutts, in case you didn't watch it already in one of the previous
links i posted ;-)
https://www.youtube.com/watch?v=AQcSFsQyct8


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9608 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar JoomliC
JoomliC - comment - 27 Mar 2016

@brianteeman Yes, an error, and i understood after your comment why, and so i just edited my message ;-)

avatar ralain
ralain - comment - 27 Mar 2016

@JoomliC The additional sanitation you suggest is probably beyond the scope of this PR. I suggest opening a new one referencing issue #7841.

I will note that filenames based entirely on a datetime could be problematic, as one can upload multiple files at the same time.

From the links you have provided it's evident that dashes are the way to go and I'll update the PR to reflect that.

avatar joomla-cms-bot
joomla-cms-bot - comment - 27 Mar 2016

This PR has received new commits.

CC: @brianteeman, @MATsxm


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

avatar JoomliC
JoomliC - comment - 27 Mar 2016
I will note that filenames based entirely on a datetime could be problematic, as one can upload multiple files at the same time.

I understand, but is there a multilple upload function on Joomla media component today ?...
And the same will apply if multiple uploads of files named for example joomla.jpg ;-)

Note: in this case, an auto-increment if file exists is not difficult to add for media manager too, as the same process as for alias.

In all cases, this is not to be included until feedback from translators.

I mention PR #7841, as i think "Why not to have norms that will suit all languages, in one shot!"...
Ok, so it seems that we have to process step by step...

  • accented characters in uploaded filename: recently fixed (so #7841 is FIXED!)
  • spaces in filename: fixed by this PR
  • Asian characters (Kanji...): not yet! (Asian users will have to wait a bit more to not have the notice "This file type is not supported.", which is of course not the appropriate alert message!). I will see to create a PR for this...

About dash to replace space: :+1:

avatar infograf768
infograf768 - comment - 27 Mar 2016

accented characters in uploaded filename: recently fixed (so #7841 is FIXED!)

By which PR?

avatar ralain
ralain - comment - 27 Mar 2016

I understand, but is there a multilple upload function on Joomla media component today ?...

Indeed, the Media Manager already allow you to select multiple files to upload in the same request.

avatar JoomliC
JoomliC - comment - 27 Mar 2016

@infograf768 this is exactly the question i had just above... The fact is in 3.5, no more issue, but #7841 is still open... and i don't find where, and when this was fixed... :-|

@ralain oh! you're right, didn't notice it before :no_mouth:
Will test this PR, as it is already an improvement for users, and a more advanced solution is not for tomorrow ;-)

avatar JoomliC JoomliC - test_item - 27 Mar 2016 - Tested successfully
avatar JoomliC
JoomliC - comment - 27 Mar 2016

I have tested this item :white_check_mark: successfully on 11c1b75


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

avatar Kubik-Rubik Kubik-Rubik - change - 29 Mar 2016
Milestone Added:
avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Mar 2016

Another question is whether spaces make sense at all in file names on the server? IMO it would be better to change the behavior globally in the makeSafe function but this could lead to a b/c break...

I agree with @JoomliC, thank you for the examples. In one of my plugins I also use underscores for the replacement of whitespaces but will change it to dashes in the next version. Additionally I also make the string lowercase.

We need another test, then it can go into the 3.5.2 version.

avatar Kubik-Rubik Kubik-Rubik - change - 29 Mar 2016
Milestone Added:
avatar Kubik-Rubik Kubik-Rubik - change - 29 Mar 2016
Milestone Removed:
avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Mar 2016

Okay, this is an easy one and I will merge it for 3.5.1. (Tests by Marc-Antoine and Brian included since only small change).
Thanks @ralain!

avatar Kubik-Rubik Kubik-Rubik - change - 29 Mar 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-03-29 11:07:42
Closed_By Kubik-Rubik
avatar Kubik-Rubik Kubik-Rubik - close - 29 Mar 2016
avatar Kubik-Rubik Kubik-Rubik - merge - 29 Mar 2016
avatar joomla-cms-bot joomla-cms-bot - close - 29 Mar 2016
avatar Kubik-Rubik Kubik-Rubik - reference | 4d8edcc - 29 Mar 16
avatar Kubik-Rubik Kubik-Rubik - merge - 29 Mar 2016
avatar Kubik-Rubik Kubik-Rubik - close - 29 Mar 2016
avatar joomla-cms-bot joomla-cms-bot - change - 29 Mar 2016
Labels Removed: ?
avatar JoomliC
JoomliC - comment - 29 Mar 2016

Another question is whether spaces make sense at all in file names on the server?
Of course, no space on server! ;-)

But users mainly have spaces in their images, pdf... on their computer. That's where this PR is really useful for a better UX.

IMO it would be better to change the behavior globally in the makeSafe function but this could lead to a b/c break...

I would better think of a new function (globalizeFilenameOnUpload...), for file renaming on upload only, which could be used for com_media, and taking care of all possible filename depending of culture. A globalized naming.
On Asian sites i've visited, i mainly found numerics as the file name, often something like this : 20160329-000010.jpg (where the last 6 digits seems to be an increment).
But i'm not Asian, so i wonder if there are norms or recommendations for such languages...

Thanks @ralain :+1:

avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Mar 2016

Sorry, I had to revert the merge because we are already in RC state and I should not have merged it.

@ralain Could you please do the same PR once again, then we can merge it into 3.5.2. Sorry for the inconvenience!

avatar ralain
ralain - comment - 29 Mar 2016

@Kubik-Rubik No problem, will do.

avatar ZoFx
ZoFx - comment - 13 Sep 2016

infograf768 this is exactly the question i had just above... The fact is in 3.5, no more issue, but #7841 is still open... and i don't find where, and when this was fixed... :-|

@JoomliC I just tested it in J3.6.2 and it's not fixed at all. Accented characters still get stripped entirely and filenames only containing accented characters result in the "filetype not supported" error thrown.

avatar Kubik-Rubik
Kubik-Rubik - comment - 14 Sep 2016

@ZoFx Can you please try my plugin "EIR" (https://joomla-extensions.kubik-rubik.de/eir-easy-image-resizer) with the option "Safe Upload Names" enabled and check whether these images are uploaded with a readable image name?

avatar JoomliC
JoomliC - comment - 14 Sep 2016

@ZoFx when testing since 3.5 with åäö.jpg, this one works, and file is renamed aao.jpg (just tested again on Joomla 3.6.2, and still works for me).
That's why i supposed this fixed, but didn't see where the change was applied...
Tested with åäöéèû.jpg and it returns aaoeeu.jpg, and file successfully uploaded.
(note: tested in com_media by using the upload button)

What does not work yet is a filename like this : 代替品.jpg
This why i proposed this in comment : #9608 (comment) (i can do a PR for asian, arabic ... characters to generate a datetime-based filename if accepted as a possible workaround ? IMO to be solved for all people from those cultures and languages!)

avatar ZoFx
ZoFx - comment - 14 Sep 2016

A similar discussion is going on in #7841 ...

@JoomliC maybe I'm completely barking up the wrong tree, but when I'm uploading files through media manager, there's no transliteration at all and åäö.jpg is refused ("filetype not supported"). I have everything updated to the latest stable (J3.6.2, com_media 3.0.0). If it matters (it proably does not ...) I'm using FF48.0.2 on Win 10. Either you or I seem to have installed an extension which influences com_media upload. I might need to test it on a complete clean install ...

@Kubik-Rubik with EIR installed in version 3.4.2 it works as described by JoomliC. åäö.jpg gets uploaded and renamed to aaeoe.jpg.

avatar ZoFx
ZoFx - comment - 14 Sep 2016

I already commented in #7841
Just to add it to this discussion as well: I just confirmed that it's not fixed in 3.6.2 stable (did a complete fresh installation). Uploading åäö.jpg through media manager results in "Notice: This file type is not supported."

avatar JoomliC
JoomliC - comment - 14 Sep 2016

@ZoFx commented here #7841 (comment) to keep messages on the right place ;-)

Add a Comment

Login with GitHub to post a comment