Bakual / SermonSpeaker

A free Joomla! component for churchs to share their sermons.
www.sermonspeaker.net
GNU General Public License v3.0
12 stars 8 forks source link

Not needed ? #31

Closed cyrezdev closed 8 years ago

cyrezdev commented 8 years ago

Hello @Bakual , Those lines seem not needed, as JFilterOutput::stringURLSafe does not use unicode option to generate slug : https://github.com/Bakual/SermonSpeaker/blob/master/com_sermonspeaker/admin/controllers/file.json.php#L78-L88

Regards, Cyril

Bakual commented 8 years ago

I think JFile::makeSafe does remove a few cases which JFilterOutput would allow (like starting with a dot). That's why it's there. The next line removes whitespaces from the filename. Neither JFile nor JFilterOutput removes them and they tend to break my stuff :smile: The last lines just make sure I don't end up with only underscores. This may not be needed anymore due to JFilterOutput, but it sure was an issue with eg russian filenames. Haven't tested that lately but leaving those lines there doesn't hurt at all.

cyrezdev commented 8 years ago

In fact, you can keep the last lines, because yes, it's ok now with special characters only (like Russian), but not if only ideograms in file (Chinese...) so, in fact, good to keep! (tested today with 日本.jpg and media manager returns invalid file... so good to keep a datetime slug, to keep filename url friendly (and so Google Images Friendly!)) :+1:

But with JFilterOutput::stringURLSafe, all dots, spaces... are replaced by dash, and at the end of function, the start and end dash if exist are removed. Exactly as generating an alias. And in all my tests today, not able to have any space or dots from the only use of this function ;-) JFilterOutput :

    public static function stringURLSafe($string)
    {
        // Remove any '-' from the string since they will be used as concatenaters
        $str = str_replace('-', ' ', $string);

        $lang = JFactory::getLanguage();
        $str = $lang->transliterate($str);

        // Trim white spaces at beginning and end of alias and make lowercase
        $str = trim(JString::strtolower($str));

        // Remove any duplicate whitespace, and ensure all characters are alphanumeric
        $str = preg_replace('/(\s|[^A-Za-z0-9\-])+/', '-', $str);

        // Trim dashes at beginning and end of alias
        $str = trim($str, '-');

        return $str;
    }

Maybe i'm wrong... but this is how i have understood stringURLSafe...

I permit myself to comment, as in fact, after an issue for one of my extension user on xp and ie7, i realized that JFile::makeSafe was... not safe! Space were kept, but not replaced by something really-url-friendly (displayed as %20), to allow to open a pdf file for example on some browser. (and found your make-sense comment here on CMS issue 7841, where having url safe filename is a nice option).

In all cases, no working issue with your code! ;-)

Bakual commented 8 years ago

You're right. JFilterOutput::stringURLSafe is indeed more restrictive than I thought. Looks like I added the JFilterPutput line afterwards and didn't check if the other lines are still needed. Thanks for the heads up :+1:

Bakual commented 8 years ago

Resolved with https://github.com/Bakual/SermonSpeaker/commit/729147797fc9c5df0db121f398f8fe4ade7edf71