Posted by webchick on May 7, 2008 at 2:25am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | file system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs Documentation |
Issue Summary
Almost every single call to file_scan_directory in core looks like this:
<?php
file_scan_directory('./profiles', '\.profile$', array('.', '..', 'CVS'), 0, TRUE, 'name', 0);
?>Wouldn't:
<?php
file_scan_directory('./profiles', '\.profile$', array('key' => 'name'));
?>...be much easier to read?
Let's improve file_scan_directory()'s usability as we did url() and l() in the 6.x cycle.
Comments
#1
#2
love the idea. subscribing.
#3
better title.
#4
I'd probably be best to wait until #74645: modify file_scan_directory to include a regex for the nomask. gets committed.
#5
YES! This function could really use this!
#6
#74645: modify file_scan_directory to include a regex for the nomask. got committed (and wrecked havoc along the way) so lets get this patch rolling again.
here's a start on it. one of the system.module tests is failing but i can't quite track down why. i haven't tried running the installer yet... odds are that it'll break that horribly.
#7
re-ran the tests and it looked good. ran through the installer and it worked fine.
we should probably add some more tests to file.test for file_scan_directory(). i'll set to needs review so the test bot can take a swing at it.
#8
Really nice. Since HEAD was broken due to this function twice in one week, would be nice to have tests alongside the patch.
#9
catch, not arguing that more tests would be good but just want to correct the record: the problems weren't with the function itself but the calling of it in install.php--which at this point is untestable.
#10
started some more unit tests and in the process found a bug in the PHPDocs. it suggested that "path", "basename", and "name" are returned when in fact it's really "filename", "basename" and "name".
more tests would be good but this is all i've got time for right now.
#11
The last submitted patch failed testing.
#12
Reroll.
#13
#14
The last submitted patch failed testing.
#15
Hmm, on my box, all File scan directory tests pass. Not sure how to debug this. The failing node test is due to #345632: SimpleTest: correct assertTitle logic.
#16
testbotbug last night, marking for retesting.
#17
I started to review this and then realized it still needed a couple of unit tests to cover all of file_scan_directory()'s options. I wrapped those up so this should be good to go.
Hopefully we can get some reviews and then get this marked RTBC?
#18
Short review of code comments:
Typos (funciton, paramter, decends, curre):
+ * Each time the funciton is called the file is stored in a static variable.+ * Check that key paramter sets the return value's key.
+ * Check that the recurse option decends into subdirectories.
+ $this->assertTrue(empty($files), t("Min-depth of 1 successfully excludes files from curre."));
JS and javascript should be "JavaScript":
+ // Grab a listing of all the JS files and check that they're passed to the+ $this->assertEqual(2, count($all_files), t('Found two, expected javascript files.'));
+ // Grab a listing of all the JS files and check that they're passed to the
// Grab a listing of all the JS files.
$this->assertEqual(2, count($all_files), t('Found two, expected javascript files.'));
+ $this->assertEqual(2, count($files), t('With recursion we found the expected javascript files.'));
Don't abbreviate things like "min":
+ * Check that the min depth lets us ignore files in the starting directory.+ $this->assertTrue(empty($files), t("Min-depth of 1 successfully excludes files from curre."));
#19
keith.smith thanks for the review... i tend to get sloppy with the unit tests. i've corrected the issues you raised.
#20
The last submitted patch failed testing.
#21
i think that was a test bot fail.
#22
The last submitted patch failed testing.
#23
no clue why that's failing. re-rolled.
#24
The last submitted patch failed testing.
#25
Got no idea why this is failing. Works fine on my machine. Dumbing down the tests a bit.
#26
The last submitted patch failed testing.
#27
#28
The last submitted patch failed testing.
#29
The testbot failures may occur, because the tests included in the patch expect file_scan_directory() to return the files in alphabetical order, but no particular sorting is guaranteed. I have added some sort() calls.
I also changed the filename masks in the tests from
/javascript*/to/^javascript-/. While not being incorrect, the former gives the impression that file_scan_directory() accepts regular filesystem wildcards instead of regular expressions.#30
great, i think the regex changes are a good idea.
well now that the test bot's happy i'd say this is RTBC.
#31
Committed to HEAD. :)
Needs to be documented in the upgrade docs.
#32
Added a note to the upgrade docs: http://drupal.org/node/224333#file_scan_directory_array-itize
#33
Automatically closed -- issue fixed for 2 weeks with no activity.
#34
The upgrade doc links to #125030: Javascript: Allow compatibility with other libraries. instead of this issue. drewish, will you fix that (I don't have access to edit that page)? At the same time, perhaps you will add the upgrade note for #380064: DX: Make file_scan_directory() use save property names as file_load() too - thanks :-)
#35
i've actually tried to add it a couple of times but had the changes disappear... think there's some problems with the book.module post d6 upgrade.
#36
I changed the link yesterday (I somehow got permission to edit the page), and the change seems to stick.
#37
Automatically closed -- issue fixed for 2 weeks with no activity.