Ignore front end vendor folders to improve directory search performance

As a note, as users of npm packages we do not have control over which files some npm packages are sent with. We also have no way of changing the node_modules folder name. In this instance... it might not be a terrible idea to prevent file_scan_directory from scanning inside a directory named "node_modules". This project name has yet to be taken on D.o, and there may be a way of safely doing so. That being said, this is an alteration of behavior and for D7 it might be better to just create a patch for those of us that are affected, and put in a full issue for D8.

(For anybody stumbling on this, I *think* if you alter line 2125 of includes/file.inc from

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

to

if (!preg_match($options['nomask'], $filename) && $filename[0] != '.' && $filename[0] !== 'node_modules) {

It should work. But I say that without any proper testing or full checking of the code. Just as a "you could try this and let me know if it works".

@afoster, I did a similar thing (noted here:https://www.drupal.org/node/2309023 ), but I added it to my package.json so it runs automatically. I also like deleting them, because I like to live life dangerously.

"scripts": {
    "postinstall": "find node_modules/ -name '*.info' -type f -delete"
  }

Edited: Quotes issue.

@iamcarrico, the only way for me, and maybe not so bad is like your #3 prevent scanning node_modules folder, but need a core update in include/file.inc
i change the nomask option in file_scan_directory function like this.

	'nomask' => '/(\.\.?|CVS|node_modules)$/',
  

I ran into the same issue. Here is my solution:

1. I created a folder in sites/all directory.
2. Then I moved all relevant files into that folder (in my case: .jshintrc, config.json, gulpfile.js, js, node_modules, package.json, sass).
3. Now you can execute `npm install`. Make sure that your minified files will be placed into site/all/theme/mysubtheme/css and /js (configure in gulpfile or in my case in config.json).

That's all.

#6 is a great way to solve this. I've attached the corresponding patch.

I have a hard time saying that #6 is a "great way" to solve this but it does solve the issue. This issue is really only for development purposes; as node_modules shouldn't be pushed to production. I think that npm should fix their issue, https://github.com/npm/npm/issues/775.

To commit this into Drupal core (7.x) would be the lazy way to solve this. There are other methods to get this done and developers should consider doing the alternatives until open source communties work together. + Drupal 8 won't have this issue as .info files are not used.

Related links (outside of Drupal):
http://stackoverflow.com/questions/16738177/grunt-possible-to-relocate-n...
http://stackoverflow.com/questions/15521655/can-i-put-the-npm-node-modul...
http://stackoverflow.com/questions/18974436/change-node-modules-location

To commit this into Drupal core (7.x) would be the lazy way to solve this

I would be a shame if Drupal required extra steps or extra knowledge just to support a workflow that frontend developers can use without a problem in other frameworks.

I'm also getting a few info files in my gem files by the way.

 find ./ -type f -name '*.info'
.//.vendor/bundle/ruby/2.0.0/build_info/breakpoint-2.4.6.info
.//.vendor/bundle/ruby/2.0.0/build_info/chunky_png-1.3.1.info
.//.vendor/bundle/ruby/2.0.0/build_info/color-schemer-0.2.8.info
.//.vendor/bundle/ruby/2.0.0/build_info/compass-1.0.1.info
.//.vendor/bundle/ruby/2.0.0/build_info/compass-blend-modes-0.0.2.info
.//.vendor/bundle/ruby/2.0.0/build_info/compass-core-1.0.1.info
.//.vendor/bundle/ruby/2.0.0/build_info/compass-import-once-1.0.5.info
.//.vendor/bundle/ruby/2.0.0/build_info/ffi-1.9.5.info
.//.vendor/bundle/ruby/2.0.0/build_info/modular-scale-2.0.5.info
.//.vendor/bundle/ruby/2.0.0/build_info/multi_json-1.10.1.info
.//.vendor/bundle/ruby/2.0.0/build_info/rb-fsevent-0.9.4.info
.//.vendor/bundle/ruby/2.0.0/build_info/rb-inotify-0.9.5.info
.//.vendor/bundle/ruby/2.0.0/build_info/sass-3.3.14.info
.//.vendor/bundle/ruby/2.0.0/build_info/sassy-maps-0.4.0.info
.//.vendor/bundle/ruby/2.0.0/build_info/singularitygs-1.2.3.info
.//.vendor/bundle/ruby/2.0.0/build_info/toolkit-2.0.0.info
.//.vendor/bundle/ruby/2.0.0/gems/ffi-1.9.5/ext/ffi_c/libffi/doc/libffi.info

I just ran up against this issue myself (got the drush error and admin areas were WSOD) with a grunt / LibSass setup for my contrib theme. It seems like a no brainer to have this added to core but in the mean time I split the difference between #4 and #5. I liked the code better in #4 so I simply added that to run in my package.json file for post install.

"scripts": {
    "postinstall": "sh npm_clean.sh"
  },

... where npm_clean.sh contains the code from #4. One thing to note, For some reason I had to add a .npmrc file that has the code unsafe-perm = true, otherwise, I was getting nasty errors. That was even running npm install as sudo npm install.

After all this, everything works great, no more drupal admin WSOD, drush works and grunt works.

First - isn't it a bug in Drush or Drupal Core that it is crashing on finding what it thinks is a malformed .info file? Rather than crash it should print an error and abort whatever it was trying to do.

Second - I can't think of a valid reason for a npm module to include something like test results in the packaged distribution. That's a sloppy package maintainer not adding things to their .npmignore file. The package maintainer should be receiving a bug on this.

Third - local modifications to a package.json will just be overwritten the next time the package is updated. Therefore the suggestions y'all made along that line will be ephemeral and break at some random time in the future.

Fourth - just because you're finding lcov.info files in this case doesn't mean some other package maintainer won't have a legitimately named a file with a .info extension.

Fifth - the idea of changing the name of the node_modules directory is a non-starter. That directory name is baked into Node core, and is the methodology it uses to find Node modules. See http://nodejs.org/api/modules.html Npm then builds on the package.json defined in the Node API to support the additional features supplied by npm.

The patch in #8 looks good as far as it goes. Seems to me there's a similar risk from bower packages containing unexpected files.

What's needed is for Drupal to (for certain purposes) simply ignore stuff inside directory structures managed by other tools like npm or bower.

#13.3: The package.json in question is at the root of the theme folder - it's not going to be overwritten by a package update. Adding a postinstall script to it is a quick, easy fix that doesn't require patching core. The only problem I see with it is your #4: someone, somewhere, someday, might add a .info file to their npm package that is actually needed.

In theory we could rewrite the code that parses .info files such that it doesn't segfault, but the fix would only apply to D7; D8 uses YAML files instead.

Yeah, I agree with @ksenzee, #13.3 won't be an issue. I've done a lot of testing around this over the past few days and codified it in a blog post here: http://dannyenglander.com/blog/drupal-drush-segmentation-fault-11-error-...

I tested this by deleting my node_modules folder and then running sudo npm install and then immediately running drush cc all. No more error. The nice thing about this method is that it's very portable across workflows and version control.

I must have missed the location of the package.json. That does render my point# 3 moot.

BTW - when using npm for a local install it's not necessary or desirable to use sudo. npm used to warn you when ran with sudo.

I like the idea of Drupal catching the error instead of crashing

Looking through the tests for drupal_parse_info_format(), it seems like its huge regex is not tested against malformed Drupal .info files.

This demonstrates the Segmentation Fault that we get with drupal_parse_info_format(). Expected to fail and kill PHP....

Pull Request and branch up at https://github.com/RobLoach/drupal/pull/3

Can be fixed in other packages to not have .info files too:

Handlebars

Will be fixed in 2.0.1: https://github.com/RobLoach/handlebars.js/commit/c5fe2527b393a1cc83948bd...

Gulp-yui

https://github.com/jsBoot/gulp-yuidoc/pull/15

lcov-parse

https://github.com/davglass/lcov-parse/pull/8

json3

https://github.com/bestiejs/json3/pull/76

I had the same problem but I only seen it when creating and updating features locally. I was able to track it down to the .info file (node_modules/grunt-eslint/node_modules/eslint/node_modules/doctrine/coverage/lcov.info) and I came across this post :). #8 fixed my problem.

Re #9 "Drupal 8 won't have this issue as .info files are not used.":
Can we be really sure? Is there a standard which says that node modules must never contain a .info.yml file? It would be a shame if have to fix the same problem again for a different file extension.

So _this_ explains why my Drupal 7 dev site was seg faulting for the last week. *sigh*

I was installing eslint and that ended up adding this file to my theme: "node_modules/eslint/node_modules/doctrine/coverage/lcov.info" And the file is zero bytes, btw.

This feels more like a major bug than a feature request. If I install the eslint tool (which Drupal 8 is using) into my theme, then Drupal seg faults for all pages.

So the next version of Zen for Drupal 7 will have people run npm install in their sub-themes. So this is a huge blocker for me.

Re #9 "Drupal 8 won't have this issue as .info files are not used.":
Can we be really sure? Is there a standard which says that node modules must never contain a .info.yml file?

We should definitely test that theory. But a random .info.yml file will still be in Yaml format, so it might get confused, but its unlikely it would segfault.

In D8, we already have tests to check for a broken .info.yml file (InfoParserUnitTest::testInfoParser), but here's a quick patch to see if we can take Rob's .info file and try the same test on it. That should verify that D8 is immune to broken .info files.

After the test bots have their way, I've got an actually-useful patch that prevents Drupal 8 from searching node_modules and bower_components folders.

I think #26 proves we don't have an issue in D8. So #2 on the proposed solution is already fixed in D8.

Here's the patch the prevents node_modules and bower_components from being traverse when looking for special Drupal files. (#1 on the proposed solution.)

In D8 this is a performance improvement (if you have node_modules in your Drupal site).

It's definitely not a major bug in D8, but it blocks a major bug in D7.

So… I'm not sure how to categorize/prioritize this. Do we leave this as "major bug"? or fork this issue to create a separate D8 task report?

Thanks for the patch, John!

To be honest I fail to see why the D8 patch blocks the D7 fix. The bug in D7 does not occur in D8 (even though it's great if we can do a related performance improvement while we're at it) so I'd say the "fix it in dev first" rule does not really apply here.

Since a chain is as strong as its weakest link, I think this issue should either remain a major bug blocked by the D8 patch, or the D8 patch should be split out into its own issue.

Regarding Drupal 7, there is already a separate issue for this - #619542: Malformed theme .info files break menu_router generation.

I'd be inclined to add the patch to that issue to cover D7, and leave this issue open for D8.

I was installing eslint and that ended up adding this file to my theme: "node_modules/eslint/node_modules/doctrine/coverage/lcov.info" And the file is zero bytes, btw.

This PR fixes it so that coverage tests arn't deployed in that package: https://github.com/Constellation/doctrine/pull/104

I'd be inclined to add the patch to that issue to cover D7, and leave this issue open for D8.

That works for me. I've RTBCed that issue.

Tweaking the title and category since this issue isn't about fixing a WSOD on D8.

Patch in #8 works fine for me in the last two months.

Shall I tag it as RTBC and against D7 so that this makes into D7 sometime soon?

@bserem there's a Drupal 7 issue open already #619542: Malformed theme .info files break menu_router generation

An rtbc should be for the latest patch and Drupal version the patch was tested against. I'm completely unsure of what you've rtbc'd here.

@alexpott, #27 is for Drupal 8, which is RTBC. It's not critical in Drupal 8, but it does speed up looking for .info files.

The Drupal 7 one you pointed to is also RTBC. They're being handled in separate issues. While the problems are similar, their solutions are different.

Well the D8 part needs a CR because if you have a module and have a sub directory called node_modules which contains a set of modules that provide node related functionality this change would break that.

+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
@@ -65,11 +65,13 @@ class RecursiveExtensionFilterIterator extends \RecursiveFilterIterator {
+    'node_modules',

Could we come up with a string that conflicts more with existing Drupal terminology? Is this a standard in node.js? Same goes for bower_components?

Also we should probably open a followup to document a module directory structure and the reserved names we currently have.

Re #40: according to http://stackoverflow.com/a/21818724, the directory name "node_modules" cannot be changed.

Since this is a bugfix for Drupal 7 I would commit it there before Drupal 8, but the unfortunate "node_modules" naming conflict gives me pause too. We could really break someone's production site with this, in order to fix a bug that (mostly?) affects developers.

Also, although the approach here is the quickest way to fix the bug, it's not necessarily the only or best one. For example, I tried these patches:
#2349457-2: preg_match_all segmentation fault on drupal_parse_info_format
#619542-68: Malformed theme .info files break menu_router generation

And with both of those applied, it fixes the problem for the .info file RobLoach provided in #19. (Haven't tested it with all the Node modules/Bower components, but it looks like it should work in general.)

In that case, this issue would just become a potential performance improvement (do we know how much of one?) for Drupal 7 too.

Related https://github.com/drush-ops/drush/issues/1445

For drush on Windows, a drush cc all will give errors for paths too long

FWIW, we have encountered the (well described in the summary) crashes also - using Compass for SASS generation.

My work-around was to hide the vendor directory in a dot-folder at install time. This excludes it from scanning.
Things seem to work fine from then on, Bundler somehow knows where to look for them next time without trouble.

bundle install --path .bundle/vendor

I'm certainly in favour of more bullet-proof scanning from the Drupal end though. Segfaulting due to a malformed info file was embarrassing.

Well the D8 part needs a CR because if you have a module and have a sub directory called node_modules which contains a set of modules that provide node related functionality this change would break that.

Change record draft is here: https://www.drupal.org/node/2544502

Is this a standard in node.js? Same goes for bower_components?

Unfortunately, yep. Both bower_components and node_modules are the default names for these folders. There's a hidden config to change Bower, but node_modules can't be changed.

Going to let the test bot review this again (since its been 3 months) before setting it to RTBC again.

Patch passes tests. https://qa.drupal.org/pifr/test/996598 Back to RTBC.

FYI, the D7 backport of this patch is over in #619542-146: Malformed theme .info files break menu_router generation and needs to be moved over here.

I'm not 100% sure this is need for D8, with this patch we only break things which are not broken at the moment. I'm unsure of what to do here.

Sounds like this still could use discussion.

I've been using the approach in #2393605: Allow file_scan_directory() to ignore folder to resolve this issue. Although manual it's a bit more generic for things like I added it to sites/all/libraries and sites/all/vendor, and to node_modules in my theme and bower_components.

#2393605: Allow file_scan_directory() to ignore folder Is the smartest approach to this problem that I've seen yet:

• It solves the seg fault issue if there are .info files in your vendor directories
• It solves the performance issue that arises when file_scan_directory() meets large/heavily nested vendor directories (this is the biggy for me personally)
• It won't break sites that require vendor directories like node_modules in production

Could we come up with a string that conflicts more with existing Drupal terminology? Is this a standard in node.js? Same goes for bower_components?

"We" didn't come up with these folder names. It's a silly coincidence, but nothing more. These folders are what is used on the front end and node_modules cannot be changed (nor should it).

I'm not 100% sure this is need for D8, with this patch we only break things which are not broken at the moment.

It doesn't break until the front end tools install them (which happens all the time when you're a front end developer).

We could really break someone's production site with this, in order to fix a bug that (mostly?) affects developers.

This is a huge "what if". If a developer knows they can group sub-modules inside a parent folder, it would be highly unlikely they would arbitrarily name it something as generic as "node_modules". Typically anything "grouped" would have a lot of context associated with it, especially if it pertained to a client, tool[set], feature, etc.

These aren't actual projects either:
https://www.drupal.org/project/node_modules
https://www.drupal.org/project/bower_components

If, on the off chance, someone actually did name a folder this.... a really simple solution: rename the folder. For the all these reasons, I cannot imagine how this could be considered even a remotely strong argument for one hypothetical site that did this. A little blurb at the top of the release notes should be sufficient way to notify anyone of this highly unlikely "potential issue".

#2393605: Allow file_scan_directory() to ignore folder Is the smartest approach to this problem that I've seen yet:

This is a nice feature, sure, but to use it as the "solution" for this issue is really just an anti-pattern. This would require documentation since developers who normally can just do npm install (and be done with it) wouldn't know about this Drupalism they would have to add to their vendor folders because Drupal will recursively look through them to find .info[.yml] files. Yes, there is a "solution" in that issue to help "automate" this via post install scripts, however that is still yet another "Drupalism".

I, for one, would much rather not have more Drupalisms to consider and constantly worry if I added this really weird "exclusion file" to my vendor dirs. Drupal should just ignore them.... period.

These folders definitely need to be added to this "blacklist". This all being said, I actually think this "blacklist" needs to be mutable. Drupal will never get this 100% right, the best we can hope for is sane defaults and allow projects to independently define blacklisted (and whitelisted) directories as needed. Perhaps through settings.php or services.yml?

100% agreed with #54. Avoiding a Drupalism is ultimately the best way to go and if we can get away with ignoring these directory names altogether then great (already doing this with https://www.drupal.org/node/619542#comment-9771891 and it works well for me on D7).

@markcarver if we go the blacklist route, then it would be nice to have it configurable to cover the cases that the .scanignore file deals with.

I don't mind the 'sane' defaults, but I'd suggest putting that in a variable_get()/Config/Setting(not sure the right one to use in D8). Then is someone does have a node_modules folder for another weird reason they can change that config, OR more likely if there is another tool that gets added to the Front End Stack, we can deal with it in the project at the time it happens.

We could probably add an update hook which throws a warning for existing sites *just in case* they have actual modules that would now be ignored due to the change. Easy enough (in Drupal 7) to scan all the existing records in the system table for anything matching system.filename LIKE '%' . db_like('/node_modules/') . '%'. I would argue D8 doesn't need this check since it's not official yet and this can be documented behavior. For existing D7 sites, the chances of this causing a problem are slim, but it doesn't mean we shouldn't do anything about it.

@joelpittet, RecursiveExtensionFilterIterator already has this black/white list (above patch). I think maybe moving them to settings.php does make a little more sense though. That way the location and method to modify these values would be the same for all core versions. That should probably be a separate issue though.

@Dave Reid, that's a really great suggestion for D7 and agree that D8 shouldn't be an issue.

edit: The D7 update should probably do file_scan_directory instead though since there could (in theory) simply be a grouping folder with no actual module associated with it (e.g. like we do with custom, contrib folders).

This hit me hard.. https://twitter.com/dcorbacho/status/632132568587051008
Not only me https://twitter.com/crizzirc/status/632138118695682049

It was not a problem of .info files... it was the fact that my node_modules has 39K files! it took extra 30 seconds to load a page.
As you can see https://monosnap.com/file/rRqDF50bipkVr949Z8Y60zxe33QCV2.png

I agree with mark carver on avoiding "another drupalism".

A temporary working fix without touching Drupal, thanks to Andrew https://www.drupal.org/u/agilmore87

chmod 700 the node_modules folder, so apache/Drupal can't access the folder.

I'm working in a virtualised (vagrant) environment with Mac as Host with NFS, so it doesn't allow me to chown the folder, but you might want to explore that route also.

So I just wasted the better part of today reinstalling my local dev stack because I thought I had messed up when I recently added a new PHP extension (xphrof). Even after re-installation, I was still getting a blank browser window and the cryptic [core:notice] [pid 57] AH00052: child pid XXXX exit signal Segmentation fault (11) messages in my Apache logs. I couldn't for the life of me figure out what I did. I even went as far as gdb httpd to figure out that it was a PCRE recursion issue, but that didn't really help pinpoint what was going on either.

It turns out I had installed two npm modules, the day before, that added the following files in the theme I was working on:

node_modules/bower/node_modules/handlebars/coverage/lcov.info
node_modules/queue/coverage/lcov.info

Because I was working soely on the theme's grunt tasks and not anything PHP related, I installed these modules many hours before refreshing the browser and noticing there was an "issue". Thus, I did not make the connection of what was actually causing the issue.

It's really ironic that even after knowing about this specific issue, someone as seasoned as myself can still run into this and it is a huge "WTF"...

So, while this isn't necessarily a "bug" in 8..x (because of YAML), it IS for 7.x... and a massive one at that. Even though there are other 7.x issues, #619542: Malformed theme .info files break menu_router generation and #2349457: preg_match_all segmentation fault on drupal_parse_info_format, they aren't necessarily related to this specific issue (crap in developer vendor directories that Drupal shouldn't care about).

So since the policy of core is to backport, we have to first "fix" 8.x before this can be properly addressed in 7.x. Even though it doesn't actually "break" anything in 8.x, it would still be a huge DX performance gain which is a win in my book.

The patch in #27 is all 8.x needs.

The "issue" about a hypothetical "node_modules" module/folder only needs to be addressed when we backport to 7.x with the update hook solution found in #57 and #58.

@markcarver I sympathise *so much* with this.

After losing $TOO_MANY_HOURS_TO_ADMIT to it a few months ago, I was recently lucky enough to be in earshot of some other (2 dev + 1 design) team members whose day had just ground to a halt and were discussing raising tickets with a serveradmin or asking for a server rebuild. Their day(s) would have been totally shot, but I had been stung hard enough to point at an innocuous 'grunt' addition the designer had added to make their image-building job easier.

That a bit of utility code (in an unsupported language!) that was only ever meant to run on demand on a designers desktop could bring the entire server to segfault is definitely this years WTF winner.

+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
@@ -65,11 +65,13 @@ class RecursiveExtensionFilterIterator extends \RecursiveFilterIterator {
+    'bower_components',
...
+    'node_modules',

I won't hold up the issue on this, but I would rather see those two directories live under a comment explaining why they are there, instead of under "Front-end". Otherwise it just looks like two random directories that someone who hasn't seen this issue may want to remove in the future.

#63 is just a personal nitpick and honestly makes no sense to me. That is why they're already under the "front-end" comment to begin with. Also, their directory names are actually self-explanatory and even if you didn't know what they were, a quick search could tell you exactly what you needed to know.

I get that core is mostly backend folk, but just because y'all don't use it doesn't make it "random". These front end technologies are actually very common and highly used (they're our "composer" if that helps any). Why do we insist on nitpicking the front end so much... can we stop please?

I just spent hours on this issue since I was getting 502 Bad Gateway on all routes under /admin on nginx.
Would be great with a proper solution, being able to blacklist node_modules folder or other.

@markcarver you seemed to misunderstand my comment about node_modules - which of course I know we did not come up with. That much is plain. But, here is the rub. Today it is bower and node, but 2 years down the line it might be something completely different. There must be other tools that add folders and files that break Drupal's "let's check everything that might possibly be Drupal" code.

The current solution looks wrong to me. The blacklist in HEAD contains concepts that Drupal creates or relies on:

  protected $blacklist = array(
    // Object-oriented code subdirectories.
    'src',
    'lib',
    'vendor',
    // Front-end.
    'assets',
    'css',
    'files',
    'images',
    'js',
    'misc',
    'templates',
    // Legacy subdirectories.
    'includes',
    // Test subdirectories.
    'fixtures',
    // @todo ./tests/Drupal should be ./tests/src/Drupal
    'Drupal',
  );

Drupal does not create or rely on the concept of bower_components or node_modules.

To me this fix looks shortsighted, in that does nothing to solve the root problem. We should fix it so the Drupal 7 does not segfault on parsing .info files. We should think about only parsing info files when there is a corresponding .module or .theme in the same folder (d7 only). We should think about a project having to supply package information that details it's sub-modules so we don't have to recursively scan everything. But continuing to add random stuff to this list will eventually break someones site in a really really hard to spot way.

Also I think if we want to quickfix it - then let's quickfix it for D7 and handle D8 in a better and more complete way.

The backport policy applies when the same issue affects both 8.x and 7.x. Since 8.x doesn't have a fatal error (or any error in fact), I think this patch could go into 7.x directly.

I'm not sure at all about the 'huge performance gain' - this is a couple of folders in a couple of modules, and a folder that shouldn't be checked into git iirc, so it's only a performance gain on dev at best.

The ability to skip sub-directories for scanning sounds useful (and agreed that'd be better than manually adding things to the list each time), just don't think we need that to fix the fatal in 7.x.

you seemed to misunderstand my comment about node_modules

No I got that you meant it sarcastically; I'm just tired of the front end being treated in a disrespectful manner. It's offensive and, quite honestly, this type of attitude towards the front end is why I had to "quit/take a break" from core.

Today it is bower and node, but 2 years down the line it might be something completely different.

The same could be said about composer. I get that compared the the back end, the front end is faster moving and could be considered by some as "unstable". However that is why tools like this came to be: to help bring stability and conformity.

I don't see node or bower going away anytime soon. This is a huge "what if". The fact of the matter is this: these tools are widely used now, even if something else comes along and we start to see a shift towards this new "technology", it wouldn't change overnight. This means that these directories would still be valid for likely many years. To say that we can't address this "what if" in the future is a complete cop-out. Please, stop arguing hypotheticals.

There must be other tools that add folders and files that break Drupal's "let's check everything that might possibly be Drupal" code.

Which is why I suggested this list get moved into settings.php in #57, however that doesn't mean we still shouldn't provide sane defaults for something very common on the front end. Based on this, I guess this issue really needs to be CNW instead of RTBC then.

Drupal does not create or rely on the concept of bower_components or node_modules.

Yes, I understand that "Drupal" itself doesn't necessarily rely on these, but core isn't just "backend" components. The fact that core provides an API for themes (theme system) means that it is aware of this extensibility. Even if core itself doesn't use these directories, it inherently touches them because of this extensibility. By adding these directories, you're in fact saying that core doesn't use them and that they should, in fact, be ignored..... which is what this whole issue is about.

But continuing to add random stuff to this list will eventually break someones site in a really really hard to spot way.

Again... not "random" and a huge "what if". There's already been a solution suggested for a way to warn any "potential" sites that may use these directories in #57 and #58.

I'm not sure at all about the 'huge performance gain' - this is a couple of folders in a couple of modules, and a folder that shouldn't be checked into git iirc, so it's only a performance gain on dev at best.

Sigh... no, this isn't necessarily a "production" performance gain, but it is a massive front end DX performance gain.

Backend developers don't see this issue because they aren't developing with these tools.

To arbitrarily dismiss this as "trivial" just because it doesn't affect you is wrong. It severely cuts into a front end developer's productivity time (see #59). We shouldn't have the opportunity to refill our coffee after initiating a cache clear.

---

I know this response may seem "harsh" or "emotional", but it isn't directed towards anyone in particular. I'm just fed up with my voice (and other front end developer voices) not being heard and our issues not being acknowledged or taken seriously. These kinds of issues are just one of the reasons why we have a low front end developer ratio in core (recommend reading the entire post when you get a chance btw).

The backport policy applies when the same issue affects both 8.x and 7.x. Since 8.x doesn't have a fatal error (or any error in fact), I think this patch could go into 7.x directly.

Also, I'm not saying that this issue should go in to "fix" the D7 bug; it would just be a happy coincidence and easy fix for info files found in the node_modules folder. This issue is simply about excluding front end vendor directories (that Drupal shouldn't traverse to begin with) to improve front end DX.

The malformed .info file bug in D7 are entirely separate issues:
#619542: Malformed theme .info files break menu_router generation
#2349457: preg_match_all segmentation fault on drupal_parse_info_format

I don't see the problem with fixing the fatal error in 7.x, which David already indicated he'd be happy to commit without an equivalent 8.x patch (since there isn't one), then handling the performance issue separately. Bundling them together does not help them get fixed faster, it just muddies the discussion as this issue shows. I'd imagine the .info fix (and even the quick fix for performance) could get into 7.x relatively quick, then we could open a separate issue for 8.x to look at performance there.

Sigh... no, this isn't necessarily a "production" performance gain, but it is a massive front end DX performance gain.

The directory scan for .info files should not be happening every request, if it is, something else is wrong. 30 seconds is indeed a long time and thanks to pointing to that comment though. Would have been useful if it was in the issue summary with a clear indication of what context the performance improvement is in (and that isn't clear even from the comment). These details are important. Also developer facing performance issues are minor relative to something that happens on production - doesn't matter whether that developer is front or back end. We should still fix them, but there's a massive difference between visitor vs. content editor vs. site builder vs. developer in terms of where the focus is on optimisation.

Also missing from this issue is an indication of how deep these directories are. I noticed in drupal_system_listing() that we don't pass any maximum depth argument to file_scan_directory(). Normally modules live at most in mymodule/modules/mysubmodule/mysubmodule.info. At least for 8.x we could enforce a maximum depth quite easily. I don't know if that would help or not because the information is not here.

To arbitrarily dismiss this as "trivial" just because it doesn't affect you is wrong

Loads of core issues don't personally affect me, whether or not they're front-end developer facing is only one factor in that - I'm much more likely to be looking at js (or preprocess/templates/CSS) than some site building interfaces during a normal work day. Just pushing back on patches is not 'disrespecting', disrespecting an issue would be ignoring it.

Right, so I would be willing to commit the current approach to Drupal 7 (with the changes mentioned in #57 and #58) as a (possibly temporary?) workaround. I would definitely prefer to see #619542: Malformed theme .info files break menu_router generation and #2349457: preg_match_all segmentation fault on drupal_parse_info_format fixed though - they are real independent bugfixes and both look pretty close to ready except perhaps for automated tests (the latter definitely needs manual testing too), because at that point this issue would just be a performance discussion for both Drupal 7 and Drupal 8 and we could focus on figuring out the best way to deal with that for both. But whatever happens, happens.

We should think about only parsing info files when there is a corresponding .module or .theme in the same folder (d7 only).

This seems interesting and I think it would work for modules, but I believe .info files are the only ones required for a Drupal 7 theme.

I will have to come back to this later to finish, but I've somewhat updated the issue summary to rectify the confusion of what this issue should be tackling.

Just pushing back on patches is not 'disrespecting', disrespecting an issue would be ignoring it.

Push back should be constructive and ask questions (as you did and I have subsequently updated the issue summary because of it).

The previous comments leading up to this point were merely speculative and rather dismissive, not "push back".

---

Also, after looking at what RecursiveExtensionFilterIterator is, I can see a little confusion here. I thought this black list was a comprehensive file_scan_directory "black list". It's not...

The patch should instead focus on adding these directories to file_scan_directory() as a default "nomask" option. As this issue is pertinent to anytime Drupal does directory/file traversal (e.g. drupal_find_theme_templates() and theme registry rebuilds).

The patch should instead focus on adding these directories to file_scan_directory() as a default "nomask" option. As this issue is pertinent to anytime Drupal does directory/file traversal (e.g. drupal_find_theme_templates() and theme registry rebuilds).

As an alternative approach, perhaps we could also just specify a custom nomask in drupal_find_theme_templates() and drupal_system_listing()?

If that approach were taken, we could further optimize the scanning of files by skipping css, js, images, and other directories as well when parsing for .tpl.php or .info files. I also like the idea of adding a nomask list that is appropriate to each situation, rather than including something specific like node_modules in the default list.

Here's a stab at the approach from #74. No change to the default nomask, but instead using a specialized nomask in drupal_find_theme_templates and drupal_system_listing().

Patch attached. Just setting to 7.x-dev for testing.

The patch in #76 looks interesting.

+++ b/includes/theme.inc
@@ -1325,8 +1325,20 @@ function drupal_find_theme_templates($cache, $extension, $path) {
+  $files = file_scan_directory($path, $regex, array('key' => 'name', 'nomask' => $no_mask));

Took me a bit to realize why you changed this line. But then noticed you want to skip 'templates' folders for system files, but not for theme hooks.

Let's have the bot have a look at it.

Oh, duh. That's a D7 patch. :-p

Yes, #76 is definitely an interesting approach. I saw this yesterday and my first impression was like "Doh, that makes a lot of sense!" However, I decided to think on this a bit and I still think putting this as a sensible default inside of file_scan_directory() makes the most sense.

I think what it it boils down to is this: because node_modules and bower_components are commonly used, they can actually appear anywhere (modules, themes, libraries, profiles, etc.). There are 22 calls to file_scan_directory() in 8.x and 26 calls in 7.x.

Since this list can change, I don't think an "individualized" approach is the right direction for something as common as these front end directories. They can appear anywhere and should always be ignored by core, which is what this issue is about. We shouldn't have to remember this each and every time a new call to file_scan_directory() is used (which will lead to future issues about this very topic again).

@JohnAlbin the 'templates' probably threw you off. ;) In D7 'theme' is also used (and perhaps should be added?), but in Backdrop we switched to 'templates' much like D8.

I like the idea of adding a nomask to file_scan_directory() because then each time it's used it can be optimized for that specific purpose. I haven't looked at all 26 calls to see how different they are, but it could be a handy tool regardless of this particular issue.

@markcarver is there a use case for a contrib project trying to include files from node_modules or bower_components using file_scan_directory?

If not, then maybe we should do both: allow file_scan_directory() to be *specifically* optimized everywhere, and always exclude node_modules or bower_components too.

is there a use case for a contrib project trying to include files from node_modules or bower_components using file_scan_directory?

Is this a possibility, sure... I suspect there may someone who may be using it this way. However this isn't really a "Best Practice™" and cannot imagine this being very commonplace. These folders are for development purposes only (e.g. to compile the files that Drupal would actually consume) in the same way that they should never be committed to the repo either.

then maybe we should do both

Yes, I think that is the most sensible approach.

My main concerns with not specifying node_modules or bower_components in file_scan_directory as sane $nomask defaults is that we will most certainly run into this issue again otherwise.

That way, when someone decides to override $nomask (as in #76), then they'll see these defaults and will likely include them along with their other specific uses cases.

We would undoubtedly have to add a comment explaining why these front end directories are sane defaults in file_scan_directory though. That way if/when someone chooses to ignore them (for whatever reason) they will know just how much it can impact local development.

Ah, I see! If $no_mask is not provided when file_scan_directory() is caled, we throw those two in. If $no_mask is specified, we assume they are included. I like it :)

Question: do we really still need the 'CVS' in there?

Question: do we really still need the 'CVS' in there?

I can't think of a reason why it would be necessary. Because CVS is no longer used for modules, it's very unlikely that a CVS directory is going to be within a module or theme directory, which is where file_scan_directory() is called most often (whether to find .info files or .tpl.php files).

The only even somewhat likely location for a CVS directory would be in the root of the project, in the event that the developer preferred to use CVS for deployment to the server or localhost development. In such a case, file_scan_directory() is never called on the *entire* installation (it's always in the modules directory, profiles directory, files directory, or themes directory), so it would never come into play. And finally, even if a CVS directory were there, this would only have a performance impact, not actually change the functionality of the site. So it's very unlikely to affect anyone negatively, and almost impossible it would actually break a site.

Following the thread over at https://github.com/backdrop/backdrop-issues/issues/1110#issuecomment-139..., I would say that we should also add scss and less to the $ignore_directories list.

While #76 and #84 both solve the issue for me (I was getting segfaults when using drush due to scanning complex node_modules directory in my theme), they also both prevent some modules from being picked up. Before the patch, my modules list was 84 deep. After the patch, my modules list was 82 deep.

The two modules that disappear are 'link_css' and 'retina_images'. It looks like the regex isn't quite right to avoid this unintended exclusion, but my regex foo isn't quite strong enough to put together a reliable solution.

This patch (D7) at least prevents the segfaults I was getting form having bower_components and node_modules in my theme, but is less aggressive in terms of what else it ignores. While I think excluding common folders like 'css' and 'images' is potentially a good thing, perhaps less is more here and we should target the core issue (node_modules/bower_components) here and worry about further performance improvements by excluding common folders (css/js/templates/images) in another thread?

Whoops, managed to bump a '?' into one of the regexes that wreaked havoc on the admin theme. Reverted back to regex pattern from #84 but without the possible string matching conflicts.

I would second the request to handle js/css/templates/js exclusion separately.
Ex: excluding templates folder completely destroys the functionality of fences module & screws up the HTML of the site

Good idea to sass/scss/less directories since they are commonly used as source folders throughout
Might be a good idea to add a /src directory as well

Thanks @kaidjohnson! I had the same problem with #84, it wouldn't detect the hires_images module.

After the patch in #89, the hires_images module was detected.

I agree with #88, we need a quickfix

perhaps less is more here and we should target the core issue (node_modules/bower_components) here and worry about further performance improvements by excluding common folders (css/js/templates/images) in another thread?

This continues being a big problem. Today I saw someone else hitting this problem https://twitter.com/illepic/status/651175874281123840 and this stackexchange question is getting very popular http://drupal.stackexchange.com/questions/126880/how-do-i-prevent-drupal...

Seems like there is some confusion about the related D7 and D8 issues. This issue relates to Drupal 8, and segmentation faults no longer occur in D8.

For the Drupal 7 issue (with a patch that is currently RTBC) please see #619542: Malformed theme .info files break menu_router generation.

As regards the Drupal 8 issue, my vote would be to just add node_modules and bower_components to the blacklist, as per the patch in #27, as long as a change record is issued to advise people to avoid using those names for modules.

Thanks, @malcomio.

I'm concerned that #619542 is a slightly different issue and while it may 'fix' the issue we're seeing, it also 'masks' the issue further. If a theme has node_modules with multiple .info files nested within, that fix will simply register those info files with a safe default name, as far as I can tell. This seems wholly unnecessary and disconcerting as we're registering files that were never intended to be a part of the Drupal system.

Here we're attempting to completely ignore and disregard entire subsets of files that are known to not be registered by Drupal. I see the symptoms being the same between these two issues, but the reasoning behind them are very different, and the solutions should both be implemented for maximum stability and minimum overhead, imo.

Not sure if it matters for this issue, but I tried D7.40 where the fix from #619542 should be included and the page still breaks with an internal server error when though the json3 nodejs module through it .info file.
Not on all pages but when deleting the cache and obviously when calling the module page...

@webcultist: See #2349457: preg_match_all segmentation fault on drupal_parse_info_format which is mentioned above.

Ah sorry, didn't mention this ticket. Thank you very much!

I read a whole thread and looks there's no consensus.

#71-72 said that issue should be fixed for D7 first so #89 looks great except "CVS" and some tuning for code comments (explain about node)

#27 patch for D8 still valid and requirement to do frontend without loosing sanity

I think we need to improve #89 get it commited and use #27 for d8 with @todo link to separate issue to make this configurable.

Hello,

Adding patch following comment #98 from andypost for drupal 7,
hope it passes testing !

suppose that ready for 7.x
change record is there, maybe we need separate for 8.x later

Updated comment style to more carefully conform to style rules.

From https://www.drupal.org/node/1354#inline:

C style comments (/* */) and standard C++ comments (//) are both fine, though the former is discouraged within functions (even for multiple lines, repeat the // single-line comment).

From some quick local testing, after one of the FE devs I work with complained of 1 minute+ cache rebuild times:

Before patch:

• Total Incl. Wall Time (microsec): 50,479,410 microsecs
• Total Incl. CPU (microsecs): 10,083,991 microsecs
• Number of Function Calls: 555,303

After patch:

• Total Incl. Wall Time (microsec): 6,508,084 microsecs
• Total Incl. CPU (microsecs): 3,721,190 microsecs
• Number of Function Calls: 416,977

It makes development sooooo much nicer for the poor FE devs who use themes that download hundreds of nested dependencies. This was on a pretty new D7 site with only about 30 modules enabled, almost nothing custom (just a bunch of features/exported content types).

What's up with the 2nd set of tests showing 3 fails? Is testbot just misbehaving?

Thanks for those benchmarks! Those are very promising results. Always nice to see a bug fix also result in performance improvements. :)

Yeah, I don't know what is with those tests... I've been using the patch on a development site for a few days now with no side effects, so I'm assuming the testbot is acting up, but I'm not 100% sure.

As I mentioned in #56 and agreed by @andypost in #98 (I think). Here is a patch that makes that patch configurable through your settings.php for your consideration.

small double space on concat nitpick fix.

+++ b/includes/common.inc
@@ -5512,12 +5512,22 @@ function drupal_system_listing($mask, $directory, $key = 'name', $min_depth = 1)
+  $ignore_directories = variable_get('ignore_directories', array(

ignore_directories seems a much too generic variable name... maybe file_scan_directory_ignore_directories instead?

Was thinking that too, maybe just drupal_ignore_directories to namespace it? But I'm fine with that really long one too. Feel free to roll that in depending how you feel.

Attached a patch that makes the variable name drupal_file_scan_ignore_directories. That way it's namespaced, and a little more specific. Since it doesn't apply globally to every file_scan_directory() invocation, it makes sense to not name it after the function itself.

Forgot to commit twice to my local branch, so there's no interdiff, but I only changed the variable name itself in the three lines/files where it was added.

Was RTBC #105, re-RTBC because the variable was just made configurable and a name change to avoid conflicts.

This should be fixed in 8.x first.

I've been working on #2609316: Port Bootstrap to Drupal 8, constantly doing cache clears and it's really starting to tick me off. Each time I do one, it's a very noticeable performance hit because I have my developer hat on where all these files are present (which is what this issue is all about).

The only reason this has been moved back to 7.x is because there was some confusion that this isn't an issue in 8.x. That is wrong. This issue is not about the malformed .info[.yml] files (that was a separate issue in 7.x and was already fixed). This issue is about the fact that core traverses unnecessary front end vendor directories.

Having to clear cache repeatedly is certainly annoying, but this is a crasher in Drupal 7, so don't think rewriting this for Drupal 8 first is good, especially when we're at RTBC for the Drupal 7 patch. I don't want to diddle the tags again (yet), but I'd really like to see this fixed in 7.

This issue is strictly about file system traversal performance (which is still an issue in 8.x and has absolutely nothing to do with parsing). The related issues (which have already been mentioned and attached to this issue) for fixing 7.x crashing are:

#619542: Malformed theme .info files break menu_router generation

#2349457: preg_match_all segmentation fault on drupal_parse_info_format

I am just adding a patch with hard-coded directories now. @geerlingguy, @markcarver, can you test and see if this makes a difference and then we can figure out how to make this configurable?

Found another usage. Please test this one.

I also read this issue again and I see that it has oscillated between D7 and D8 several times. I am not sure where we stand right now but I am posting the patch anyway.

I definitely conflated this with the related issues when I first looked at it, so agreed on 8.x assuming the problem is the same

D8 Before patch:

• Total Incl. Wall Time (microsec): 10,431,715 microsecs
• Total Incl. CPU (microsecs): 4,072,000 microsecs
• Number of Function Calls: 883,587

D8 After patch:

• Total Incl. Wall Time (microsec): 2,950,189 microsecs
• Total Incl. CPU (microsecs): 1,964,000 microsecs
• Number of Function Calls: 634,441

Difference: 111% faster cache clear after patch, ~250,000 fewer function calls

This was with Drupal 8 HEAD (as of today), with and without the patch in #117 above. For flexibility's sake, I would like to see those two options as configurable options in Drupal 8 (like they are in the D7 patch in #111), so setting back to needs work for that.

I tested with the same theme directory node_modules and bower_components directories that I had in the D7 tests above.

I think this is more generic and may cover off the config need. Hopefully didn't overstep my bounds. @hussainweb let me know if that works for you?

And hopefully does the trick still @geerlingguy?

Oh and I removed (\..*)| because it's covered with the $filename[0] != '.' I figured

@joelpittet: Not at all. :) Thank you for the changes. It is still missing in RecursiveExtensionFileIterator, and we should use it there as well for the sake of consistency. I don't think we should directly use Settings here, but inject it. I will see if I can attempt a solution later today.

@hussainweb thanks. I agree with the Settings injection if possible and hopefully I got the wrapping syntax on long conditionals correct because I don't do that often.

Not sure if we need the configuration on RecursiveExtensionFileIterator but if you find a way to do it well, all the power to ya;)

I have been using this patch for a few days now and works perfectly. I am using Vagrant for development which is affected by this extremely. Page load times after a cache rebuild decreased from 60s to 10s.

Here is my attempt to use Settings in RecursiveExtensionFilterIterator. But with this, I am afraid that I am changing API and this might go to 8.1.x. Can we commit the patch in #121 to 8.0.x and this patch to 8.1.x? :P

@hussainweb maybe a postponed follow-up issue for 8.1.x changes?

Sounds reasonable.

I created a follow-up for 8.1.x at #2640572: Allow extending the blacklist in RecursiveExtensionFilterIterator to improve directory search performance. We can review the patch in #121 here to keep things simple.

I've done another performance test.

Scenario: I installed basic theme, npm install the modules it comes with.

Apply this little microtime test patch and clear the cache on the command line via drush cr.
Apply the patch in #121
and clear cache again.

No Patch
---------
0.07743501663208
0.526780128479 <---- THIS IS HUGE!
0.00018978118896484

With Patch
---------
0.069730997085571
0.010643005371094 <---- :-)
0.00014495849609375

Here's the above performance test with npm install for Bootstrap 8.x-3.x-dev:

No Patch
---------
0.049306869506836
0.30480408668518 <--
0.00015616416931152

With Patch
---------
0.041584014892578
0.0044867992401123 <--
0.00018906593322754

+++ b/core/includes/file.inc
@@ -953,7 +964,10 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
+          && !(isset($options['nomask']) && preg_match($options['nomask'], $filename))
+          && !(!empty($default_nomask) && preg_match($default_nomask, $filename))

We should only provide a default "nomask" if one wasn't already provided.

As it stands now, this code will always exclude the "default nomask". It should extend (+=) the existing $options['nomask'] array with the defaults instead.

I have a vanilla Drupal 8.0.2 install and use the Basic theme after "npm install" and "bower install".

Added #131 for performance testing and did a "drush cr" before and after the patch #121:

Before

0.0304069519043
0.170646905899
0.000127077102661

After

0.0295081138611
0.00653886795044
0.000136852264404

I have a vanilla Drupal 8.0.3 install and use the Basic theme after "npm install".

Added #131 for performance testing and did a "drush cr" before and after the patch #121:
No patch
0.026458978652954
0.20892095565796
0.00011992454528809

After patch
0.019454002380371
0.0019950866699219
7.4863433837891E-5

Hopefully addresses #133. This doesn't effect the performance at all because that function doesn't get called for modules/themes discovery anymore.

Reviewed the code and it looks good. We have still 2 needs tags that needs to be solved before this is RTBC.

I know people are using as am I the d7 patch in production. Plus the performance tests should count for as much extra too.

If someone is near a computer to update the issue summary, please do

Read through the issue summary, it seems good to me. The tag was added in #73 and updated in #120

+++ b/core/includes/file.inc
@@ -945,6 +946,19 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
+    $ignore_directories = Settings::get('drupal_file_scan_ignore_directories', [

This should be documented in one of the settings.php examples.

Otherwise looks good though.

Patch with added description and example to default.settings.php

Thanks @andriyun, good start :)

+++ b/sites/default/default.settings.php
@@ -705,6 +705,19 @@
+ * The list of folders disabled for directory scanning.
+ * For example we added node_modules and bower_components folders to avoid
+ * issues with Drupal recursive scanning.
+ * Uncomment below strings to enable.

I would say "Uncomment the below strings to enable", that makes it a bit less terse. However it's also a bit misleading IMO to say they are enabling anything when the default is the same as uncommenting.

I also think this either needs to be combined into one paragraph or split into multiple paragraphs, the way the lines are bunched together doesn't match with our docs standards or the rest of default.settings.php:

https://www.drupal.org/node/1354#general

To make a paragraph break in a docblock, leave a blank line (see example above).

Added adjustments to description:

• Removed summary line b/c first line of text contains the essence of description.
• Added mention about default value.
• Added blank line before last to make more focus on uncomment action.

+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
@@ -71,6 +71,8 @@ class RecursiveExtensionFilterIterator extends \RecursiveFilterIterator {
     'templates',
+    'node_modules',
+    'bower_components',
     // Legacy subdirectories.
     'includes',

would be great to add line comment here about where that comes from

@andypost how about
// Deeply nested Node.js folders.

@joelpittet great, just for consistency with other ones in the list

Thanks, there we go.

Thanx! Let's get this in

I don't think we can ignore existing sites and BC, unfortunately. It is perfectly possible to have created a directory in modules called node_modules with a load of custom modules to do with node integration in it. Such a site would be broken by this change.

In order to get around this we need to add the setting to settings.php without it being commented out and the default value in code should be an empty array. That way new sites get the new settings but old sites are not broken.

Another problem is that the setting makes this look extensible but in fact it is not...

@@ -71,6 +71,9 @@ class RecursiveExtensionFilterIterator extends \RecursiveFilterIterator {
+    // Deeply nested Node.js subdirectories.
+    'node_modules',
+    'bower_components',

Hardcoding here... shouldn't we be getting the new setting and adding it to the blacklist?

@alexpott: We are using a blacklist in #2640572: Allow extending the blacklist in RecursiveExtensionFilterIterator to improve directory search performance which targets 8.1.x. Since using a blacklist properly would mean a change on the interface, we didn't do that for 8.0.x. See comments #127 to #130.

Adding it as a default in settings.php rather than inline makes sense to me.

@hussainweb there's no change to any interface - but I'm pretty certain that this is not something we'd do in a patch release considering the nature of the change - it is a task not a bug.

Sorry, I don't know why I said interface. We would need to be able to inject the blacklist in the RecursiveExtensionFilterIterator which (kind of) changes the constructor. I agree that can't be done in a patch release and that is why there is another issue for 8.1.x. See the patch in #2640572-2: Allow extending the blacklist in RecursiveExtensionFilterIterator to improve directory search performance.

The patch in this issue does not break anything (in the API) but unfortunately has to hardcode the directories. Calling Settings::get from inside the iterator seems like a smell. Theoretically speaking, yes, it is possible that there would be a directory called node_modules with custom modules but somewhat unlikely. In any case, it is possible and it counts as a break. Maybe we could remove it from the iterator object in the patch here (for 8.0.x) and get some performance gains at least. We go all the way in the other issue which is for 8.1.x. Thoughts?

I don't think we can ignore existing sites and BC, unfortunately. It is perfectly possible to have created a directory in modules called node_modules with a load of custom modules to do with node integration in it. Such a site would be broken by this change.

The odds of this actually occurring seem astronomically microscopic considering how, we as a community, generally think and name things using "Best Practices". The name "node_modules" has never been a "thing" in Drupal, even as a remote concept AFAIK and was and has been only ever introduced as a known FE "thing".

I find this argument extremely weak and hypothetical. Just because something is possible doesn't always make it a reality. It would be far different if there was a previous core module, naming convention, or published contrib module... none of which has happened.

If this truly is a concern, the solution is simple: warn the one or two hypothetical users that have done this about the change in an update hook, as mentioned in the IS (I personally still think this is overkill).

Also, this really isn't an "API break" anyway. Any "fix" would be relatively simple: rename the very hypothetical folder/module.... boom, done.
This change would not actually affect any logic inside the very hypothetical folder/module (which is what an "API break" really is about).

@hussainweb well then this patch should remove all configurability because having it configurable in one place and not another does not make sense. Let's just combine the changes and do this in 8.1.x - it is around the corner and the correct place to make non-bug fixes anyway.

@markcarver trading a broken site against a slow cache clear is not a hypothetical comparison. This can change can break sites. Please stop telling me this is hypothetical. Unfortunately we allow our folder structures for discovery extensions to be a complete wild west. But also note, I've note been saying don't make this change I've been trying to help this change go in in a way that will not break existing sites - so, exactly what is the problem?

All I'm suggesting is that we do this. And we need to test the new setting probably by extending Drupal\system\Tests\File\ScanDirectoryTest for file_scan_directory() changes and we need to add a test for the ExtensionDiscovery changes somewhere...

Also I don;t think we need to prefix the setting with drupal_ this is a Drupal setting after all.

Here's the test for file_scan_directory(). Still need a test for ExtensionDiscovery

Please stop telling me this is hypothetical.

I wasn't saying it towards you specifically. I was using it to point out that issue keeps slanting towards a tone that has no real merit.

So, no. I'm not going to "stop" using that word, especially when it's being used quite appropriately:

That this issue can, or rather, might break a site. That statement isn't fact. It's bolstering a "what if" scenario that has no real basis on truth, whatsoever, other than the coincidental name of the folder. There is a minute possibility (if not, complete improbability) that some individual decided (in 8.x no less which has far fewer contrib development than 7.x btw) to name their module or group modules in a sub-folder with the same names?

Really?

I've seen stuff go in far more damaging than this, often without any perceivable foresight into the damage it "might" cause. That fact that this issue has already identified this scenario and provided a viable "solution" (update hook, see IS) is a far better deal than other core issues that had no trouble "breaking" the FE left and right in the name of "progress" (or "tasks" I should say). In fact, it actually astounds me to see what core picks and chooses as a "BC break".

It's hypocritical, a double standard and quite frankly... complete rubbish.

---

I'm not going to argue anymore about this. I have repeated myself multiple times now, with clearly no ability to convey English in a comprehensible manner.

So, since 8.1.x is "just around the corner", then let's just stop the bikeshedding and taking things personally. The only reason this issue is on 8.0.x is because of the backport policy.

---

When we do backport this issue finally to 7.x, it will still need some sort of mechanism to warn users of ignored folders/modules that might have the name. I thought It made sense to figure that out here/now... but I guess it's being decided that it's now "out-of-scope" until we backport?

+++ b/core/includes/file.inc
@@ -946,6 +947,13 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
+  $default_nomask = '/^' . implode('|', $ignore_directories) . '$/';

@@ -954,7 +962,10 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
+        if ($filename[0] != '.'
+          && !(isset($options['nomask']) && preg_match($options['nomask'], $filename))
+          && !(!empty($default_nomask) && preg_match($default_nomask, $filename))
+          ) {

This re-introduces the bug where $default_nomask is always applied even if an explicit $options['nomask'] value was provided, see #133.

+++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
@@ -90,6 +87,20 @@
+  public function __construct(\RecursiveIterator $iterator, array $blacklist = []) {
+    parent::__construct($iterator);
+    $this->blacklist += $blacklist;
+  }

Doesn't this constitute an API break? If you see the patch in the issue for 8.1.x, this is exactly what we do there (because the function signature changes).

Oh, I see you changed this issue to 8.1.x-dev. Do you mean we shouldn't really try this for 8.0.x?

Side-question: What happens to 8.0.x after 8.1.0 comes out? Will it be supported for bug-fixes for some time?

Yes, that's what seems to be the consensus. I was going to change it to 8.1.x, just for the sake of stopping the the "argument". I really don't care when/how this is done... just that it's done so we can move on to the 7.x backport.

re #161 mmm that means the patch on #2640572: Allow extending the blacklist in RecursiveExtensionFilterIterator to improve directory search performance was wrong - which shows the important of having a test. Which this patch now has for this.

Given that my tone has no real merit, I guess my addition of tests has no real merit either. So I'm not going to add the test for ExtensionDiscovery because I don't want to spend my time doing that. But a test does need to be added before this should be committed.

o_O

I did not say that you or your tone "has no real merit". I said [this] issue keeps slanting towards it when it's really not necessary, especially given the solutions already provided and the context for what this change implies (and its actual reality of impact). Ok.. done.

---

Re: #2640572: Allow extending the blacklist in RecursiveExtensionFilterIterator to improve directory search performance
I think that issue should be closed since this one is now 8.1.x. It's only adding to the confusion, especially considering that work has been make on the above patches (in this issue) after that other issue/patch was created.

@hussainweb once 8.1.x comes out 8.0.x is unsupported - see https://www.drupal.org/core/release-cycle-overview#minor

@alexpott: Thanks! I think then there is no point trying a backward compatible way for 8.0.x. 8.1.x it is.

+1 on this. It's good to make it configurable, and putting it in settings.php means it can be backported and D7 benefits as well.

+++ b/sites/default/default.settings.php
@@ -705,6 +705,17 @@
 /**
+ * The list of folders disabled for directory scanning.
+ *
+ * By default ignore node_modules and bower_components folders to avoid issues
+ * with Drupal recursive scanning.
+ */
+$settings['file_scan_ignore_directories'] = [

Using settings is a good idea, but then we have to explain it to the user better. This comment and maybe other comments need to explain the scope of the ignore. It's not obvious what's Drupal scans and what it doesn't, so what's an end user to think?

Maybe something like "Will ignore directories with this name in any directory scan, such as finding extensions." and also add @see file_scan_directory and @see ExtensionDiscovery.

As no one else wrote the test I decided too and hey look the changes to RecursiveExtensionFilterIterator didn't work. Patch attached fixes that and tests it.

Unrelated random test fail due to #1993928: Language of parts: Introduce a language toolbar button

Looks perfect for me.
tests, docs, proper comments
Thank you @alexpott
+1 to RTBC

1. Not in patch, file_scan_directory documentation:

    *   - 'nomask': The preg_match() regular expression for files to be excluded.
    *     There is no default.
   

   This seems out of date now, there is in fact a default.

2. +++ b/core/includes/file.inc
   @@ -946,6 +947,15 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
   +    $default_nomask = '/^' . implode('|', $ignore_directories) . '$/';
   

   Does this need to be escaped? We should be able to trust settings files but it is not clear from the settings documentation that it is a regex you're providing not a directory name. IMO we should document that it is a regex, or we should preg_quote().

3. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -433,11 +433,17 @@ protected function scanDirectory($dir, $include_tests) {
   +    // Allow directories specified in settings.php to be ignored. You can use
   +    // this to not check for files in common special-purpose directories. For
   +    // example, node_modules and bower_components. Ignoring irrelevant
   +    // directories is a performance boost.
   +    $ignore_directories = Settings::get('file_scan_ignore_directories', []);
   +
   

   Globals are arguments so we are hiding a API change to ExtensionDiscovery::scan() here. I tried to come up with a suggestion for clarifying it but ExtensionDiscovery is quantifiably complex. Maybe it is just ok? Maybe we make the blacklist an argument to scan() similar to file_scan_directory? I don't know.

4. +++ b/sites/default/default.settings.php
   @@ -705,6 +705,17 @@
   + * The list of folders disabled for directory scanning.
   + *
   + * By default ignore node_modules and bower_components folders to avoid issues
   + * with Drupal recursive scanning.
   + */
   

   This seems less clear than it could be. This provides the default list of directories that will be ignored by Drupal's file API's, it is only hard coded to disable scanning in ExtensionDiscovery as mentioned in the previous bullet.

Thanks for the review @neclimdul

1. Fixed - good catch
2. Went for the preg_quote - I don't think we should be supporting regexes in the file_scan_ignore_directories setting. That would open all sorts of interesting ways to break stuff.
3. Yeah it's not nice - ideal I think there should be an ExtensionDiscoveryFactory that can take care of injecting what we need - but this is okay for now - it is not too bad to set up a settings object to test this - or change at will.
4. Fixed.

8.2.x automated testing was turned off - uploading the patch again...