Updated with #89.

Problem/Motivation

The file_scan_directory() function includes a default nomask pattern that is redundant. It excludes the CVS directory which is now long time history and therefore very unlikely to be used.

Proposed resolution

Remove the default nomask.

Remaining tasks

None

User interface changes

None.

API changes

It is possible that a specific contrib module that was relying on the default nomask pattern will have to supply a custom one.

CommentFileSizeAuthor
#90 file_scan_directory-nomask-1165694-90.patch2.77 KBSutharsan
#90 interdiff-1165694-89-90.txt817 bytesSutharsan
#89 file_scan_directory-nomask-1165694-89.patch2.68 KBSutharsan
#89 interdiff-1165694-86-89.txt1.35 KBSutharsan
#86 file_scan_directory-nomask-1165694-86.patch2.67 KBSutharsan
#81 file_scan_directory-nomask-1165694-81.patch3.02 KBpillarsdotnet
#80 file_scan_directory-nomask-1165694-80.patch3.12 KBpillarsdotnet
#78 file_scan_directory-nomask-1165694-78.patch3.14 KBpillarsdotnet
#76 file_scan_directory-nomask-1165694-76.patch3.12 KBpillarsdotnet
#73 file_scan_directory-nomask-1165694-73.patch3.12 KBpillarsdotnet
#70 file_scan_directory-nomask-1165694-69.patch3.18 KBpillarsdotnet
#64 file_scan_directory-exclude_dot_dirs-1165694-64.patch7.83 KBpillarsdotnet
#64 file_scan_directory-include_dot_dirs-1165694-64.patch7.63 KBpillarsdotnet
#61 file_scan_directory-exclude_dot_dirs-1165694-62.patch7.73 KBpillarsdotnet
#61 file_scan_directory-include_dot_dirs-1165694-62.patch7.53 KBpillarsdotnet
#62 file_scan_directory-exclude_dot_dirs-1165694-63.patch7.83 KBpillarsdotnet
#62 file_scan_directory-include_dot_dirs-1165694-63.patch7.63 KBpillarsdotnet
#60 file_scan_directory-exclude_dot_dirs-1165694-61.patch7.34 KBpillarsdotnet
#60 file_scan_directory-include_dot_dirs-1165694-61.patch7.15 KBpillarsdotnet
#59 file_scan_directory-exclude_dot_dirs-1165694-59.patch7.39 KBpillarsdotnet
#59 file_scan_directory-include_dot_dirs-1165694-59.patch7.15 KBpillarsdotnet
#54 file_scan_directory-exclude_dot_dirs-1165694-54.patch6.77 KBpillarsdotnet
#54 file_scan_directory-include_dot_dirs-1165694-54.patch6.77 KBpillarsdotnet
#53 file_scan_directory-include_dot_dirs-1165694-53.patch6.16 KBdroplet
#53 file_scan_directory-exclude_dot_dirs-1165694-53.patch6.14 KBdroplet
#52 file_scan_directory-exclude_dot_dirs-1165694-52.patch6.81 KBpillarsdotnet
#52 file_scan_directory-include_dot_dirs-1165694-52.patch6.81 KBpillarsdotnet
#50 file_scan_directory-exclude_dot_dirs-1165694-50.patch6.78 KBpillarsdotnet
#50 file_scan_directory-include_dot_dirs-1165694-50.patch6.77 KBpillarsdotnet
#46 file_scan_directory-exclude_dot_dirs-1165694-46.patch6.78 KBpillarsdotnet
#46 file_scan_directory-include_dot_dirs-1165694-46.patch6.76 KBpillarsdotnet
#40 file_scan_directory-exclude_dot_dirs-1165694-40.patch6.86 KBpillarsdotnet
#40 file_scan_directory-include_dot_dirs-1165694-40.patch6.76 KBpillarsdotnet
#37 file_scan_directory-exclude_dot_dirs-1165694-37.patch6.76 KBpillarsdotnet
#37 file_scan_directory-include_dot_dirs-1165694-37.patch6.69 KBpillarsdotnet
#34 file_scan_directory-exclude_dot_dirs-1165694-34.patch6.64 KBpillarsdotnet
#34 file_scan_directory-allow_dot_dirs-1165694-34.patch6.56 KBpillarsdotnet
#33 file_scan_directory-exclude_dot_dirs-1165694-33.patch6.75 KBpillarsdotnet
#33 file_scan_directory-allow_dot_dirs-1165694-33.patch6.67 KBpillarsdotnet
#30 file_scan_directory-exclude_dot_dirs-1165694-30.patch6.2 KBpillarsdotnet
#30 file_scan_directory-allow_dot_dirs-1165694-30.patch6.26 KBpillarsdotnet
#28 file_scan_directory-exclude_dot_dirs-1165694-28.patch6.17 KBpillarsdotnet
#28 file_scan_directory-allow_dot_dirs-1165694-28.patch6.23 KBpillarsdotnet
#25 file_scan_directory-exclude_dot_dirs-1165694-25.patch6.19 KBpillarsdotnet
#25 file_scan_directory-allow_dot_dirs-1165694-25.patch6.25 KBpillarsdotnet
#22 file_scan_directory-no-mask-defaults-1165694-22.patch5.72 KBpillarsdotnet
#15 file_scan_directory-nomask_defaults-1165694-15.patch2.22 KBpillarsdotnet
#4 file_scan_directory-nomask_defaults-1165694-4.patch2.16 KBpillarsdotnet
#1 file_scan_directory-nomask_git-1165694-1.patch858 bytespillarsdotnet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pillarsdotnet’s picture

pillarsdotnet’s picture

Status: Active » Needs review
droplet’s picture

Status: Needs review » Needs work

- need to change function doc as well
- consider remove CVS or also add .svn, .hg, the common version control folders..

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
2.16 KB

Added a subset of the exclusions used by rsync in its cvs-exclude mode, and changed docs to match.

pillarsdotnet’s picture

Title: file_scan_directory() should include .git in its default nomask option. » file_scan_directory() should include common version-control and temporary files in its default 'nomask' pattern.
pillarsdotnet’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

Aron Novak’s picture

What about the infamous __MACOSX directory? Maybe it should be excluded as well.
Otherwise, the patch seems to be great.

pillarsdotnet’s picture

I fail to understand why a __MACOSX directory would be deliberately included within a drupal website, and if it were, why we would want to exclude it by default.

I originally got involved in this issue because the automatic scans of my .git folders were killing performance, and I was unwilling to delete them. It is my understanding that __MACOSX folders, if they exist, have only a few small files.

Aron Novak’s picture

They can be copied to files directory by any user by accident. AFAIK some contrib modules suggest you to copy files/directories directly into /files/ for later processing - that's why it can appear in the filesystem.
Those folders can be big or not, not sure if it is false or not: "Some take as much as 30% of the file." - from:
http://superuser.com/questions/104500/what-is-macosx-folder

Personally i've seen such folders under drupal site's files folder, the question if they can grow big or not actually.
Why we should exclude it by default? Cause it's a metadata what would even not appear if the originating ZIP is extracted under macosx.

pillarsdotnet’s picture

Whatever. Nobody is going to RTBC this anyway.

pillarsdotnet’s picture

Everett Zufelt’s picture

+1 on the concept. Patch applies cleanly on 8.x.

Sadly not familiar enough w/ regex to give a +1 on implementation.

Comment seems a bit verbose, can we summarize what is included (cvs, git, bzr, temp, etc... and let those who care look at api.d.o?

pillarsdotnet’s picture

Issue tags: +Needs tests

Sadly not familiar enough w/ regex to give a +1 on implementation.

I suppose I should write some unit tests to prove that it works, such as creating a directory structure containing at least one of each excluded type, plus at least one of each commonly-included type, and test that a file_scan_directory() over the directory tree returns only the expected results.

Comment seems a bit verbose, can we summarize what is included (cvs, git, bzr, temp, etc...

That is exactly what the comments are intended to summarize.

and let those who care look at api.d.o?

And that is exactly where the comments would be published.

Really, I don't understand what, if anything, you are objecting to here. Can you provide a patch, or at least an annotated quote, to illustrate?

Everett Zufelt’s picture

* The default includes the following:
* - Current, parent, CVS, RCS, SCCS, SVN, GIT and BZR directories. Temporary file prefixes and suffixes

Basically I don't see a need to document all the regex patterns in the doc block when they are in the code.

pillarsdotnet’s picture

Status: Needs review » Needs work

Fine. CNW until I get around to rolling another patch, as well as unit tests.

But honestly, I've kinda lost hope of this ever making it into core.

Everett Zufelt’s picture

I definitely think it should be in core, but maintainers only look at issues like this when crit / maj bugs and tasks are under their thresholds. * note I realize that this has been sitting around for a while.

With tests and the doc block change I would be happy to set RTBC.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
5.72 KB

Interesting. I find that even with $options = array('nomask' => '/^NOMASK$/'), directories beginning with a leading '.' are excluded from file_scan_directory() results.

Setting CNR for testbot anyway.

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-no-mask-defaults-1165694-22.patch, failed testing.

droplet’s picture

This line from file_scan_directory excluded '.' (All file start with DOT)

if (!preg_match($options['nomask'], $filename) && $filename[0] != '.') {
pillarsdotnet’s picture

Thanks; dunno how I missed that. A little digging reveals that this feature was added to exclude .svn directories.
See #91592: file_scan_directory() scans subversion working base (.svn) by default

Gonna upload two different strategies; y'all tell me which one looks better.

pillarsdotnet’s picture

Status: Needs work » Needs review

CNR for bot.

pillarsdotnet’s picture

Status: Needs work » Needs review

Just looked at those test results in detail. WTF is "dummy-remote" ??!!!!

pillarsdotnet’s picture

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-allow_dot_dirs-1165694-28.patch, failed testing.

pillarsdotnet’s picture

Trying again.

pillarsdotnet’s picture

Posted testbot bug report at http://drupal.org/node/1282144

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-allow_dot_dirs-1165694-30.patch, failed testing.

pillarsdotnet’s picture

Whoops! The current and parent directories must *always* be excluded.

pillarsdotnet’s picture

Better tests -- removed a couple of unnecessary asserts and drupal_realpath() calls.

pillarsdotnet’s picture

Issue tags: -Needs tests

Removing tag since tests are written and working.

droplet’s picture

Status: Needs review » Needs work

It's few comments

+++ b/includes/file.incundefined
@@ -2004,17 +2004,25 @@ function file_download() {
+    . '|CVS(\.adm)?' // CVS directories.

It matched: xCVSx

+++ b/includes/file.incundefined
@@ -2004,17 +2004,25 @@ function file_download() {
+    . '|RCS(LOG)?|SCCS' // RCS and SCCS directories.

Same as above, will match xRCSx..etc

+++ b/includes/file.incundefined
@@ -2004,17 +2004,25 @@ function file_download() {
+    . '|\.(bzr|darcs|git|svn)' // Other version-control directories.

mercurial is quick popular now. I remember the temp is .hg ?
(http://mercurial.selenic.com)

+++ b/includes/file.incundefined
@@ -2025,7 +2033,11 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
+      if (!preg_match($options['nomask'], $filename)) {

I'm not 100% sure if PHP will do it automatically. We don't need capture back here. so our regex can be (?:xxx), performance optimized. (a little bit increase only I think)

10 days to next Drupal core point release.

pillarsdotnet’s picture

@droplet -- I see that you are reviewing the alllow patch rather than the exclude patch. Is that the version you prefer?

It matched: xCVSx ... Same as above, will match xRCSx..etc

I disagree, and will add tests to prove it.

Mercurial ... .hg

Thanks; will add this.

Our regex can be (?:xxx), performance optimized

Yup; will do.

pillarsdotnet’s picture

Status: Needs work » Needs review
droplet’s picture

Status: Needs work » Needs review

ahh. I can see it now. I missed this 2 => "^" "$".

It's quick hard to read.

can we rewrite it to:

$nomask = '/
^(?:
\..* 					              # Anything starting with a dot.
|CVS(?:\.adm)? 			              # CVS directories.
|RCS(?:LOG)?|SCCS 		              # RCS and SCCS directories.
|(?:cvslog\.|\.?\#|,|_\$|\.del-).*  	      # Temporary file prefixes.
|.*?(?:~|\$|\.(?:old|bak|BAK|orig|rej)) # Temporary file suffixes.
|__MACOSX 				      # MacOSX resource fork directory.
)$
/x';

I found an example. I think the code standard allowed.
http://api.drupal.org/api/drupal/includes--common.inc/function/filter_xss/8

*** I will try to figure out the different between 2 patch later, maybe tmr ***

pillarsdotnet’s picture

It's quick hard to read.

I think you mean "quite" hard to read?

Can we rewrite it to ...

Can do. Hopefully @sun will chime in and tell me whether I'm violating his coding standards.

*** I will try to figure out the different between 2 patch later, maybe tmr ***

I'm sorry.

  1. The exclude patch ignores all filenames starting with a dot.
  2. The include patch only ignores specific dot-filenames:
    • .bzr
    • .darcs
    • .hg
    • .svn

Status: Needs review » Needs work
Issue tags: -git deployment

The last submitted patch, file_scan_directory-include_dot_dirs-1165694-40.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
pillarsdotnet’s picture

Status: Needs review » Needs work
Issue tags: +git deployment

The last submitted patch, file_scan_directory-include_dot_dirs-1165694-40.patch, failed testing.

droplet’s picture

I voted Exclude all .dot dirs

Okay. One more problem I've seen.

+++ b/includes/file.incundefined
@@ -2004,17 +2004,26 @@ function file_download() {
+      |(?:cvslog\.|\.?#|,|_\$|\.del-).*       # Temporary file prefixes.
+      |.*?(?:~|\$|\.(?:old|bak|BAK|orig|rej)) # Temporary file suffixes.

These TWO can be optimized

suffixes

patch regex:

^.*?(?:~|\$|\.(?:old|bak|BAK|orig|rej))$

I suggested:

(?:~|\$|\.(?:old|bak|BAK|orig|rej))$

It will not attempt to match at character 0,1,2,3,4 to end.

e.g.
abcdefgh.rej

patch regex attempt to match:
a
ab
abc...
......
.............
abcdefgh.
abcdefgh.r
abcdefgh.re
abcdefgh.rej
(until find it or fail)

New one:
a (failed. and move to next)
(SKIP)
. (matched now)
.r
.re
.rej

prefixes

patch regex:

^(?:cvslog\.|\.?#|,|_\$|\.del-).*$

I suggested :

^(?:cvslog\.|\.?\#|,|_\$|\.del-).

It will stop after matched the pattern.

OLD:
cvslog.F
cvslog.FI
cvslog.FIL
cvslog.FILE

NEW:
cvslog.F
(bingo !!)

I'm not explain very well but hope it make sense.

Well done @pillarsdotnet. Thanks!!

Oh. 04:17 am on my side. Time to bed :)

10 days to next Drupal core point release.

pillarsdotnet’s picture

@droplet -- like this?

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-include_dot_dirs-1165694-46.patch, failed testing.

boombatower’s picture

Status: Needs work » Needs review

sub.

mac specific ignore --

boombatower’s picture

Status: Needs review » Needs work

cross-post

pillarsdotnet’s picture

Fixed -- switching to extended regex requires that I backslash-escape the embedded '#'.

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-include_dot_dirs-1165694-50.patch, failed testing.

pillarsdotnet’s picture

@droplet -- Took me a long time to figure out how to use your methods while combining prefixes, suffixes, and whole-name matches into a single regex,

pillarsdotnet’s picture

Issue summary: View changes

Revise using issue summary template.

droplet’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary: View changes

Updated links to point to latest patch files.

pillarsdotnet’s picture

Issue summary: View changes

Update links to latest patch versions.

droplet’s picture

Status: Needs review » Needs work
+++ b/includes/file.incundefined
@@ -2004,17 +2004,27 @@ function file_download() {
+      |^(?:cvslog\.|\.?\#|,|_\$|\.del-).       # Temporary file prefixes.

@pillarsdotnet
my one with a DOT at the end. I assume all prefixes will contain at least one more char ? e.g.: cvslog.a

Without it, im happy with the CODE part now :)

10 days to next Drupal core point release.

pillarsdotnet’s picture

Ah. So "cvslog." should be included, while "cvslog.a" should be excluded, in your opinion?

I suppose -- Should I include that distinction in the tests?

droplet’s picture

Status: Needs work » Needs review

@pillarsdotnet,

I rethink again. it do not necessarily.

pillarsdotnet’s picture

Status: Needs review » Needs work

Well, I already rewrote the code and tests -- if they pass locally, I'm going to upload.

pillarsdotnet’s picture

Tests pass locally, and the updated test code is both simpler and more robust, I think.

pillarsdotnet’s picture

Issue summary: View changes

Separation of the two points of contention.

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary: View changes

Updated patch links and added two dot-prefixes.

pillarsdotnet’s picture

Moved to a separate test case. Since I can't get 'dummy-remote' to create files, there's no sense in adding this test to the "File API (remote)" section.

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary: View changes

Updated patch links.

pillarsdotnet’s picture

Issue summary: View changes

Updated patch links again.

Everett Zufelt’s picture

Status: Needs review » Needs work

+ * - 'nomask': The preg_match() regular expression for files to excluded.

files to be excluded.

Looks good to me other wise.

pillarsdotnet’s picture

Facepalm!

Everett Zufelt’s picture

Status: Needs review » Reviewed & tested by the community

This is looking pretty good to me.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/file.inc
@@ -2004,17 +2005,26 @@ function file_download() {
+      .(?:~|\$|\.(?:old|bak|BAK|orig|rej))$ # Temporary file suffixes.
+      |^(?:\.|cvslog\.|\#|,|_\$).           # Hidden or temporary prefixes.

Almost all of these look unnecessary to me. Users of file_scan_directory() are supposed to pass in an explicit mask that does not yield false-positive backup files in the first place. This means that .old, .bak, .orig, .rej files are not in collected anyway.

.git, .svn, and .whatever are excluded already now, since we do not recurse into hidden directories (starting with a dot).

The other prefixes and suffixes look like big assumptions to me, which don't necessarily hold water. And in combination with aforementioned point, should not be found by the scan in the first place.

+++ b/includes/file.inc
@@ -2004,17 +2005,26 @@ function file_download() {
+        CVS(?:\.adm)?                       # CVS directories.
+        |RCS(?:LOG)?|SCCS                   # RCS and SCCS directories.

Not sure whether it makes sense to introduce these RCS/LOG/SCCS/.adm cases at this point in time. 6 years ago, that might have been a good idea. But adding them today is pretty much pointless.

+++ b/includes/file.inc
@@ -2004,17 +2005,26 @@ function file_download() {
+        |__MACOSX                           # MacOSX resource fork directory.

This addition to nomask is more or less the only in this patch that might make sense. But highly debatable on its own, as many would consider it a user mistake/error to upload these directories.

9 days to next Drupal core point release.

pillarsdotnet’s picture

Status: Needs work » Closed (works as designed)

Of course @sun is right, as always. Sorry I wasted everybody's time.

droplet’s picture

Status: Closed (works as designed) » Needs work

@pillarsdotnet,

Sorry I wasted everybody's time.

No. :)

something I think we can do here:
- enable it to able recurse into dir start with DOT?

+++ b/includes/file.incundefined
@@ -2004,17 +2005,26 @@ function file_download() {
-    'nomask' => '/(\.\.?|CVS)$/',

we can remove CVS now?
\.\. should be removed since a DOT is always exclued.

+++ b/includes/file.incundefined
@@ -2025,7 +2035,11 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
-      if (!preg_match($options['nomask'], $filename) && $filename[0] != '.') {

check $filename[0] != '.' first, better performance.

8 days to next Drupal core point release.

sun’s picture

enable it to able recurse into dir start with DOT?

What's the use-case of doing that? Never heard of anyone wanting to recurse into hidden subdirectories in the past 5 years. Normally, hidden directories are hidden on purpose, not arbitrarily.

\.\. should be removed since a DOT is always exclued.

. is, but .. is not.

check $filename[0] != '.' first, better performance.

That assumes that there'd be more hidden files/directories than non-hidden, which I don't think is the case (except for, perhaps, in your user home directory). But of course, if you can prove that the other order is faster, then I'll happily shut up. ;)

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.18 KB

The only thing else left to do is to remove the nomask pattern entirely.

droplet’s picture

Status: Needs review » Needs work

$filename[0] != '.'

$filename = .
$filename = ..

$filename[0] will be . DOT, no?

at least this one can be rewrite from

\.\.?
to
\.\.

droplet’s picture

Issue summary: View changes

Minor spelling/grammar correction.

pillarsdotnet’s picture

Title: file_scan_directory() should include common version-control and temporary files in its default 'nomask' pattern. » Remove CVS from file_scan_directory default nomask pattern.
FileSize
3.12 KB

The attached patch hardcodes the exclusion of dot-files, making the nomask option unnecessary.

If that is undesirable, I leave it up to you experts to debate which of the following is fastest:

  1. (!preg_match('/^\.\.?/', $filename) && !preg_match($options['nomask'], $filename))
  2. (!preg_match($options['nomask'], $filename) && !preg_match('/^\.\.?/', $filename))
  3. ($filename !== '.' && $filename !== '..' && !preg_match($options['nomask'], $filename))
  4. (!preg_match($options['nomask'], $filename) && $filename !== '.' && $filename !== '..')
pillarsdotnet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-nomask-1165694-73.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.12 KB

Fixed typo.

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-nomask-1165694-76.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.14 KB

Okay, have to insert an isset() check.

Status: Needs review » Needs work

The last submitted patch, file_scan_directory-nomask-1165694-78.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
3.12 KB

(sigh)

pillarsdotnet’s picture

Make docs match code.

MaxWesten’s picture

#81 seems to work like a charm....
Site working with .svn much much faster.

sun’s picture

Title: Remove CVS from file_scan_directory default nomask pattern. » Remove default nomask from file_scan_directory()
Category: bug » task
Issue tags: -git deployment +Performance

If this is about performance now, then we need benchmarks.

I can't believe this change makes a big difference. If it really does, then we might gain a similar improvement by front-anchoring the default nomask PCRE (with '^').

xjm’s picture

Issue tags: +needs profiling
xjm’s picture

Issue summary: View changes

Entire focus changed by direction of @sun.

areke’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The proposed patch no longer applies, so it will need to be re-rolled.

Sutharsan’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Performance, -needs profiling, -Needs reroll
FileSize
2.67 KB

Rerolled patch from #81.

Since #1808200: Unit test performance significantly decreased since system list config conversion already changed the content of the default 'nomask' and introduced the more efficient filename checking using $filename[0] !== '.', this issue is no longer about performance improvement. Removed the corresponding tags accordingly. The patch now only removes the default value of 'nomask'.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me now.

The code of file_scan_directory() was copied into Drupal\Core\SystemListing::scanDirectory(), but the offending default $nomask does not exist in that copy.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
+++ b/core/includes/file.inc
@@ -1190,7 +1187,7 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
+        if ($filename[0] !== '.' && !(isset($options['nomask']) && preg_match($options['nomask'], $filename))) {

is the comparison operation !== necessary? I don't think it is. We're not using this in SystemListing::scanDirectory()

Also since the scope of the patch has changed we need an issue summary update.

Sutharsan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.35 KB
2.68 KB
Sutharsan’s picture

The above reverts a change in the documentation of the function introduced in previous patches and changes 'period' into 'dot'. Further the comment of #88 is processed.
This one fixes a typo in the comment.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +API change

Thanks! Summary appears to have been updated as well.

The last submitted patch, 89: file_scan_directory-nomask-1165694-89.patch, failed testing.

alexpott’s picture

Title: Remove default nomask from file_scan_directory() » Change notice: Remove default nomask from file_scan_directory()
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change, +Needs change record

Committed 9d5e802 and pushed to 8.x. Thanks!

xjm’s picture

Issue tags: +Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

Gábor Hojtsy’s picture

Title: Change notice: Remove default nomask from file_scan_directory() » Remove default nomask from file_scan_directory()
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Added/published change record at https://drupal.org/node/2188553

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.