Skip to content

Loading…

get_dir_file_info() the return array's key needs to be unique #92

Open
bitbucket-import opened this Issue · 9 comments

8 participants

@bitbucket-import

The get_dir_file_info() function in the file helper has a bug in the return array being generated.

$file is being used as the return arrays key. But this in not unique enough in that If I have 2 similarly named files in different folders, the first files data is overwritten by the secong eg.

I have the following files:

assets\bingopizza_dbn\language\english\sitetext_lang.php

assets\bingopizza_dbn\language\spanish\sitetext_lang.php

The english files data is missing.

If I call the function as per below:

get_dir_file_info('assets\bingopizza_dbn\language\', $top_level_only = FALSE)

The result returned is:

Array
(
[sitetext_lang.php] => Array
(
[name] => sitetext_lang.php
[server_path] => ...assets\bingopizza_dbn\language\spanish\sitetext_lang.php
[size] => 31
[date] => 1285930111
[relative_path] => ...assets\bingopizza_dbn\language\spanish\
)
)

@ckdarby

Confirmed with this on current develop branch

@rsanchez

I'd love to see a solution to this, perhaps using the server_path as the array key. That would break backwards compatibility, and I'm not sure what the protocol is for making pull requests that break BC.

@Indigo74

I stumble into the exact same problem. But there is an easy solution using CI extending capability:

(1) create a "MY_file_helper.php" in 'application/helpers' ("MY" depending on your configuration)
(2) open the original "file_helper.php" in 'system/helpers' and copy the whole get_dir_file_info() function
(3) paste it into your "MY_file_helper.php" (without the if (function_exists()) stuff)
(4) now we can peacefully modify the behaviour. Replace those two lines :

$_filedata[$file] = get_file_info($source_dir.$file);
$_filedata[$file]['relative_path'] = $relative_path;

by :

$result = get_file_info($source_dir.$file);
$result['relative_path'] = $relative_path;
$_filedata[] = $result ;

(5) enjoy!

This is pretty straightforward actually. Instead of using $file as a key, I use a temporary variable ($result) to store the data and then I add it to the file array. A unique key is automatically given by using []. The result now looks like :

Array
(
0 => Array
(
[name] => test.php
[server_path] => ...application\language\french\test.php
[size] => 2
[date] => 1348057318
[relative_path] => ...application\language\french\
)
1 => Array
(
[name] => test.php
[server_path] => ...application\language\english\test.php
[size] => 2
[date] => 1348057318
[relative_path] => ...application\language\english\
)
)

Hope this helps someone.

@jim-parry

The solution proposed clearly breaks backwards compatibility.
What do you think of an additional function, get_dir_files_info, with the same parameter list but returning all of the nested files?
I realize this would need an update to the file_helper documentation and the changelog.

@Razican

The problem is that even if it breaks backwards compatibility, currently it is broken.

@jim-parry

Even if broken, the proposed fix changes the nature of the returned array. The original is index by filename, whereas the proposal isn't indexed (positional only).
Perhaps the answer is to add get_dir_files_info and to point out in the docs for get_fir_file_info that the answer might not be what was expected, even if it is backwards-compatible.

@Razican

Or we could change the behaviour and write the changes in the upgrade guide.

@mwhitneysdsu

I think that get_dir_file_info is still useful when you know the limitations from the start, and would certainly be faster, in situations where it is appropriate, than what I am about to propose. For that reason, I think it would be best to document the limitation of get_dir_file_info and create another solution for the problem (which should also be noted in the documentation of get_dir_file_info).

My proposal would be to add a parameter to directory_map in the directory_helper which would indicate that the map should include the output of get_file_info for each file. So, if this optional parameter were set, the output of directory_map would look something like this:

Array 
(
    [assets] => Array
    (
        [bingopizza_dbn] => Array
        (
            [language] => Array
            (
                [english] => Array
                (
                    [sitetext_lang.php] => Array
                    (
                        [name] => sitetext_lang.php
                        [server_path] => ...assets\bingopizza_dbn\language\english\sitetext_lang.php
                        [size] => 31
                        [date] => 1285930111
                        [relative_path] => ...assets\bingopizza_dbn\language\english\
                    )
                )
                [spanish] => Array
                (
                    [sitetext_lang.php] => Array
                    (
                        [name] => sitetext_lang.php
                        [server_path] => ...assets\bingopizza_dbn\language\spanish\sitetext_lang.php
                        [size] => 31
                        [date] => 1285930111
                        [relative_path] => ...assets\bingopizza_dbn\language\spanish\
                    )
                )
            )
        )
    )
)

This would also be relatively easy to implement, changing line 91 of directory_helper from

$filedata[] = $file;

to

$filedata[] = $new_optional_param ? get_file_info($file) : $file;

and adding something like

if ($new_optional_param === true)
{
    get_instance()->load->helper('file');
}

to the beginning of the function.

Of course, much of the output becomes less necessary with the extra info supplied by the structure of the directory_map output, so there are many other potential considerations, like accepting yet another optional parameter (or making the new parameter a mixed false/array()) to pass as the second parameter for get_file_info to tell it what information to return.

@mwhitneysdsu mwhitneysdsu added a commit that referenced this issue
@mwhitneysdsu mwhitneysdsu #92 `directory_map()` alternative for `get_dir_file_info()`
- Added optional parameter to `directory_map()` to call
`get_file_info()` on each file and add the result to the function's
output.
- Documented limitation of `get_dir_file_info()` when multiple files in
the directory hierarchy share the same name, and recommended use of
`directory_map()` as a possible solution.

This is not necessarily a fix for #92, but a work-around to maintain
backwards compatibility (in both functions) and make it relatively easy
to gain access to this information under these conditions.
227dfa1
@gadelat

My opinion is that this helper should not use filenames as keys. Accessing filename from array is easy enough even without this "feature". It should be changed to use numeric keys as is proposed by @Indigo74 . Yes, it will be BC-break. I still think this should be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.