? Success

User tests: Successful: Unsuccessful:

avatar marenkov
marenkov
24 Sep 2016

JFile::getExt works incorrect if file does not have extension.

avatar marenkov marenkov - open - 24 Sep 2016
avatar marenkov marenkov - change - 24 Sep 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Sep 2016
Category Libraries
avatar joomla-cms-bot joomla-cms-bot - change - 24 Sep 2016
Labels Added: ?
avatar Fedik
Fedik - comment - 24 Sep 2016

Please add description how to reproduce the issue, and how to test the fix.

avatar marenkov
marenkov - comment - 24 Sep 2016

You can not reproduce this problem in the CMS, only in an extension. But you can write php-code that uses it: $ext = JFile::getExt ('test');
Result: $ext = "est'
Right result: $ext = ''

avatar Minei3oat
Minei3oat - comment - 24 Sep 2016

Maybe a false is a better return value, if there is no extension. is returned if there is a dot as last letter in the string. So false would indicate that there is no dot.

avatar Minei3oat
Minei3oat - comment - 24 Sep 2016

I've tested JFile::getExt('test.'). It returns false, but it should be .

avatar ggppdk
ggppdk - comment - 24 Sep 2016

I confirm the issue and the fix,
@marenkov , Joomla code must follow specific code styling

if ($dot === false)
{
    return ''; 
}

About

Maybe a false is a better return value, if there is no extension. is returned if there is a dot as last letter in the string. So false would indicate that there is no dot.

Filename with trailing dot and without extension is usually not valid,
although some programs might create it, e.g. if you create such a name in windows it will be difficult to use or delete it ...

avatar zero-24
zero-24 - comment - 24 Sep 2016

What about

    /**
     * Gets the extension of a file name
     *
     * @param   string  $file  The file name
     *
     * @return  string  The file extension
     *
     * @since   11.1
     */
    public static function getExt($file)
    {
        return strtolower(pathinfo($file, PATHINFO_EXTENSION));
    }

Based on: https://github.com/joomla-framework/filesystem/blob/master/src/Stream.php#L243

avatar Minei3oat
Minei3oat - comment - 24 Sep 2016

Nearly perfect, but there is the code style issue...
The following line condenses your two lines to one and solves the code style issue:

return $dot === false ? false : (string) substr($file, $dot + 1);
avatar ggppdk
ggppdk - comment - 24 Sep 2016

Recently i was trying to remember why i was not using

JFile::getExt

and instead i am using:
pathinfo($file, PATHINFO_EXTENSION)

@zero-24
just, we cannot use strlower(), because

  • it will be change of behaviour for the method
  • it is also incorrect to force it, the caller should get the true extension without any lowercasing, the caller will do it if needed, (current code does it already when needed)

about

return $dot === false ? false : (string) substr($file, $dot + 1);

it seems good, but better not rediscover the wheel, for no good reason,
instead just use:

return pathinfo($file, PATHINFO_EXTENSION);
avatar zero-24
zero-24 - comment - 24 Sep 2016

Thanks @ggppdk good spot. I agree with return pathinfo($file, PATHINFO_EXTENSION); instead of the current code ;)

avatar marenkov
marenkov - comment - 24 Sep 2016

return pathinfo($file, PATHINFO_EXTENSION); looks good, but it returns string(0) for 'test.' and 'test' (instead false).
If it is not problem, it is the best solution.

avatar Minei3oat
Minei3oat - comment - 24 Sep 2016

@ggppdk please use ```php instead of ```, because with the php the code is highlighted, wich makes it easier to read :)

avatar ggppdk
ggppdk - comment - 24 Sep 2016

@ggppdk please use php instead of, because with the php the code is highlighted, wich makes it easier to read :)

Done, thanks, i guess, i need to recheck github documentation

avatar Minei3oat Minei3oat - test_item - 24 Sep 2016 - Tested successfully
avatar Minei3oat
Minei3oat - comment - 24 Sep 2016

I have tested this item successfully on 7c236a4


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

avatar ggppdk ggppdk - test_item - 24 Sep 2016 - Tested successfully
avatar ggppdk
ggppdk - comment - 24 Sep 2016

I have tested this item successfully on 7eab124


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

avatar zero-24 zero-24 - change - 24 Sep 2016
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 24 Sep 2016

RTC


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

avatar joomla-cms-bot joomla-cms-bot - change - 24 Sep 2016
Labels Added: ?
avatar rdeutz
rdeutz - comment - 25 Sep 2016

Sorry guys, but I don't think we should do it this way. I don't like the different return types

JFile::getExt('whatever')

should return an empty string

JFile::hasExt('whatever')

should return true or false (we don't have such a function atm and I am not saying we should make one :-))

avatar rdeutz rdeutz - change - 25 Sep 2016
Status Ready to Commit Needs Review
Labels Removed: ?
avatar rdeutz
rdeutz - comment - 25 Sep 2016

set to needs review


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

avatar ggppdk
ggppdk - comment - 25 Sep 2016

@rdeutz

don't like the different return types
should return an empty string

Ok then, lets not make 2 changes in this PR, (1 fix + 1 behaviour change)

Let it return string as before, and ... only fix the original reported bug:

  • Lets make it return empty string when no extension exists (= the original reported bug)
// Check for no extension
if ($dot === false)
{
    return '';
}
avatar marenkov
marenkov - comment - 26 Sep 2016

In anyway current realization is wrong. We have two version of fix:
My version - we need select what is returned for files without extension and with dot at the end.
pathinfo - always returns string(0) if extension is empty. Not bad.
Somebody should select and fix it.

avatar rdeutz
rdeutz - comment - 26 Sep 2016

I made a decision as maintainer, return an empty string instead of false

avatar marenkov
marenkov - comment - 27 Sep 2016

OK. I've changed false to empty string.

avatar rdeutz rdeutz - change - 27 Sep 2016
Status Needs Review Ready to Commit
avatar rdeutz
rdeutz - comment - 27 Sep 2016

Thanks back to RTC


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

avatar joomla-cms-bot joomla-cms-bot - change - 27 Sep 2016
Labels Added: ?
avatar rdeutz rdeutz - change - 27 Sep 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-09-27 15:06:06
Closed_By rdeutz
avatar rdeutz rdeutz - close - 27 Sep 2016
avatar rdeutz rdeutz - merge - 27 Sep 2016
avatar joomla-cms-bot joomla-cms-bot - close - 27 Sep 2016
avatar joomla-cms-bot joomla-cms-bot - change - 27 Sep 2016
Labels Removed: ?
avatar zero-24
zero-24 - comment - 27 Sep 2016

?

Add a Comment

Login with GitHub to post a comment