Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Almost every single call to file_scan_directory in core looks like this:
file_scan_directory('./profiles', '\.profile$', array('.', '..', 'CVS'), 0, TRUE, 'name', 0);
Wouldn't:
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.
Comment | File | Size | Author |
---|---|---|---|
#29 | file_255551_8.patch | 20.48 KB | c960657 |
#25 | file_255551.patch | 18.42 KB | drewish |
#23 | file_255551.patch | 17.61 KB | drewish |
#19 | file_255551.patch | 17.63 KB | drewish |
#17 | file_255551.patch | 17.41 KB | drewish |
Comments
Comment #1
webchickComment #2
drewish CreditAttribution: drewish commentedlove the idea. subscribing.
Comment #3
drewish CreditAttribution: drewish commentedbetter title.
Comment #4
drewish CreditAttribution: drewish commentedI'd probably be best to wait until #74645: modify file_scan_directory to include a regex for the nomask. gets committed.
Comment #5
Dave ReidYES! This function could really use this!
Comment #6
drewish CreditAttribution: drewish commented#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.
Comment #7
drewish CreditAttribution: drewish commentedre-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.
Comment #8
catchReally nice. Since HEAD was broken due to this function twice in one week, would be nice to have tests alongside the patch.
Comment #9
drewish CreditAttribution: drewish commentedcatch, 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.
Comment #10
drewish CreditAttribution: drewish commentedstarted 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.
Comment #12
c960657 CreditAttribution: c960657 commentedReroll.
Comment #13
c960657 CreditAttribution: c960657 commentedComment #15
c960657 CreditAttribution: c960657 commentedHmm, 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.
Comment #16
catchtestbotbug last night, marking for retesting.
Comment #17
drewish CreditAttribution: drewish commentedI 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?
Comment #18
keith.smith CreditAttribution: keith.smith commentedShort review of code comments:
Typos (funciton, paramter, decends, curre):
JS and javascript should be "JavaScript":
Don't abbreviate things like "min":
Comment #19
drewish CreditAttribution: drewish commentedkeith.smith thanks for the review... i tend to get sloppy with the unit tests. i've corrected the issues you raised.
Comment #21
drewish CreditAttribution: drewish commentedi think that was a test bot fail.
Comment #23
drewish CreditAttribution: drewish commentedno clue why that's failing. re-rolled.
Comment #25
drewish CreditAttribution: drewish commentedGot no idea why this is failing. Works fine on my machine. Dumbing down the tests a bit.
Comment #27
drewish CreditAttribution: drewish commentedComment #29
c960657 CreditAttribution: c960657 commentedThe 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.Comment #30
drewish CreditAttribution: drewish commentedgreat, i think the regex changes are a good idea.
well now that the test bot's happy i'd say this is RTBC.
Comment #31
webchickCommitted to HEAD. :)
Needs to be documented in the upgrade docs.
Comment #32
drewish CreditAttribution: drewish commentedAdded a note to the upgrade docs: http://drupal.org/node/224333#file_scan_directory_array-itize
Comment #34
c960657 CreditAttribution: c960657 commentedThe 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 :-)
Comment #35
drewish CreditAttribution: drewish commentedi'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.
Comment #36
c960657 CreditAttribution: c960657 commentedI changed the link yesterday (I somehow got permission to edit the page), and the change seems to stick.