Collecting database file operations

[dreadnaut]

Originally reported on Google Code with ID 208

The code relative to the $database array is spread a bit here and there: main flow,
dir_tree(), checkDbName(), isManagedDb...

I've tried to collect *most* of the relative operations in a class representing a list
of database files. Attached is a partial version, I still need to add read/write checks
and some lists.

The functions above are now methods: addDirectory, canCreate, contains. There are also
more ways to fill the set: a single filename, a directory, and the current configuration
format.

The second file attached is a small example/test:
  trunk/classes/DatabaseSet.php
  trunk/test-dbs.php

Any feedback or ideas around this part of the code?

Reported by dreadnaut on 2013-04-04 19:16:17

<hr> * Attachment: DatabaseSet.php * Attachment: test-dbs.php

[Christopher Kramer]

In general: Yes, good idea to bring this together in a class. And good implementation,
especially using realpath().

What I noticed:
1. allowed_extensions: Why public & no setter? I think some write protection to this
would be good as it is security relevant. Maybe it should only be possible to set it
using the constructor.


2. addDirectory: It's a matter of taste, but I think I would have implemented this
a bit differently. Not that your implementation is bad at all. Just my thoughts: 

First, I prefer to implement recursive stuff recursively. Why building up a stack yourself
if the language can do it for you? Okay, might be more efficient.

Second, I would use DirectoryIterator instead of glob('*'). Mainly because it's more
readable. (And glob() has such ugly notes in the documentation: "Note: This function
isn't available on some systems (e.g. old Sun OS)." - this sounds like it is still
safe on most systems, but could also mean it isn't.)

Third, I would call addFile() from addDirectory, mainly for readability and maintainability
(e.g. realpath() only once in the code). Your solution is probably more efficient,
though.

Fourth, I don't see a good reason for $new_files. Why not directly writing to $this->_files?
In rare cases, you might overwrite some $this->_files[realpath($f)] with another "unreal"
filename pointing to the same file. Who cares? Why is the first "unreal" filename better
then the second? Why do you keep the "unreal" paths at all?

As I said, just my thoughts. Your code is fine, if you don't feel like discussing,
just go ahead ;-)

Reported by crazy4chrissi on 2013-04-26 20:54:01

[Christopher Kramer]

(Maybe I shouldn't start too many discussions like this about 20 LOC methods that do
what they are supposed to only because I would have implemented them differently...)

Reported by crazy4chrissi on 2013-04-26 20:58:05

[dreadnaut]

I like these discussions, usually better code comes out of them :)

I'd love to use DirectoryIterator (and RecursiveDirectoryIterator, and RecursiveIteratorIterator
to avoid recursion or queues) but I find them actually less readable, and they feel
to expensive for what we are doing. Things get a bit better with FilesystemIterator,
which however requires 5.3.0+.

For example, RecursiveDirectoryIterator allows you to specify the SKIP_DOTS flag, while
DirectoryIterator doesn't —so long, code reuse. At that point we need to call ->isDot()
anyway to check —at that point, let's stick to opendir(). Plus, a bunch of objects
allocated to list a few files, each one opening and closing a file handle —vs opening
the directory and calling stat() a few times.

Readability, er... starting to look like Java :D

try {
    if ($recursive) {
        $iter = new RecursiveIteratorIterator(new DirectoryRecursiveIterator($path, FilesystemIterator::SKIP_DOTS));
    } else {
        $iter = new DirectoryIterator($path /* sorry, no flags accepted here */);
    }
} catch (UnexpectedValueException $e) {
    return false;
}

foreach ($iter as $file) {
    $filename = $file->getFilename();
    /* same code anyway */
}

If glob is a problem, I'd probably use opendir() at that point. The SPL is usually
nice, but I'm not sold on these classes.

Apart from this, I've changed the interface a bit compared to the initial code, and
I can now accept these formats:

/* configuration examples */
$databases_example = array (

// a single database
    'dir/storage.sqlite',

// a single database, with an alias
    'ShortName' => 'mystuff/database_with_a_long_name.sqlite',

// a single database, with an alias (long form, current configuration style)
    array(
        'path' => 'directory/something.sqlite',
        'name' => 'My Something DB',
    ),

// specific files in a directory
    'some_directory/*.{sqlite,db}',

// all the files in a directory
    'some_directory/',

// all the files in a directory, recursively
    array(
        'path' => 'just_a_directory/',
        'recursive' => true,
    ),

);

But I was wondering: which other operations would we need?  Does it make sense to include
deleting/renaming files here?

Reported by dreadnaut on 2013-05-01 16:28:19