Ignore node_module folder in core to use Drupal with npm/grunt/nodejs

Please provide a proper patch: https://www.drupal.org/node/707484

Duplicate of #2329453

@jeroen.b, that is actually a D8 issue.

This is for D7.

It's marked for Drupal 7 backport, though.

Adding an reroll here to not confuse the d8 issue.

In light of https://www.drupal.org/node/2715157 I assume this should be reopened and marked as postponed until #2329453: Ignore front end vendor folders to improve directory search performance is fixed?

#2329453: Ignore front end vendor folders to improve directory search performance is now fixed.

The patch still applies. Are there any other tasks here?

The patch in #11 is incomplete, imo. There are three places where this functionality happens in D7. Also, node_modules isn't enough. bower_components is just as likely to exist (maybe less so now than a year ago), but should be considered a standard front-end module folder.

Migrating final D7 patch (https://www.drupal.org/node/2329453#comment-10622208) from #2329453: Ignore front end vendor folders to improve directory search performance here for review.

This should be hitting a variable_get('file_scan_ignore_directories'); to retrieve the defaults to keep parity with Drupal 8.

The newest drupal 7 patch in #2329453 is https://www.drupal.org/node/2329453#comment-10637754 (#111 not #101).

Added that patch here (2 files are equal):
- ignore_front_end_vendor-2329453-111.patch <-- from #2329453
- ignore_front_end_vendor-2482549-19-7x.patch <-- renamed file
@#17 this patch has the variable_get

Both the patches in #20 have test runner passes as of Dec 7, 2016 (after they apparently failed). Moving back to CNR.

The tests passed on June 7th, so given this is a backport of functionality already in D8 what's the recommended process to get this committed? Do we need a subsystem maintainer to review?

Can confirm that this patch works for me. node_modules is no longer a blight upon my loading times.

It is not necessary to repeat the following code 3 times in 3 different places

  // By default, do not check for files in common special-purpose directories.
  // The folders here are front-end related and they have been added to avoid
  // issues with Drupal recursive scanning. In this case, we added node_modules
  // and bower_components. This also improves performance on frontend builds.
  $ignore_directories = variable_get('drupal_file_scan_ignore_directories', array(
    'node_modules',
    'bower_components',
  ));
  $no_mask = '/^(\..*)|' . implode('|', $ignore_directories) . '$/';

The only place that needs the code above is file_scan_directory() in file.inc.

I will explain why:
If you call file_scan_directory() but you don’t provide the array of options containing ‘nonask’, file_scan_directory() will add it to the $options array, see

// Merge in defaults.
$options += array(
 'nomask' => $no_mask,
 'callback' => 0,
 'recurse' => TRUE,
 'key' => 'uri',
 'min_depth' => 0,
);

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

We do not need to change the code above. We do not know what side effects we might have by changing the call from drupal_find_theme_templates() to file_scan_directory().
Besides, if you look into drupal_find_theme_templates(), it does a call to file_scan_directory() anyway.
For the same reasons as I explained on part I, if file_scan_directory() gets called without $options[‘nomask’], it will use the default options.

Lastly, for consistency, added the default values to default.settings.php

$conf['file_scan_ignore_directories'] = array(
  'node_modules',
  'bower_components',
);

So the actual patch can be really small.
I am adding a new patch with some tests for it.

The patch will look a lot more similar to what has been done in Drupal 8 on https://www.drupal.org/node/2329453

Unrelated failure

Re-submitting the patch

confirming that #31 patch works as expected.

The patch works as expected! Looks pretty similar to what we have in D8.

It makes life a lot easier in case if local development uses NFS file sharing and npm is used by some modules/themes.

Patch is working fine, thanks, we really need this!

1. +    $ignore_directories = variable_get('file_scan_ignore_directories', array(
   +      'node_modules',
   +      'bower_components',
   +    ));
   

   Unless I'm missing something, that's a big difference compared to the Drupal 8 code - Drupal 8 uses an empty array as the default value (and therefore ensures backwards compatibility by not forcing the behavior to change for existing sites; see #2329453-150: Ignore front end vendor folders to improve directory search performance). Why wasn't that done here?

2. +    array_walk($ignore_directories, function (&$value) {
   +      $value = preg_quote($value, '/');
   +    });
   

   This code will break on PHP 5.2.

   I think a simple foreach ($ignore_directories as &$ignore_directory) would probably be easier to understand anyway.

3. +    });
   +    $default_nomask = '/^(\.\.?)|CVS|' . implode('|', $ignore_directories) . '$/';
   +  }
   +
      // Merge in defaults.
   .....
   @@ -2143,7 +2157,10 @@ function file_scan_directory($dir, $mask, $options = array(), $depth = 0) {
      $files = array();
      if (is_dir($dir) && $handle = opendir($dir)) {
        while (FALSE !== ($filename = readdir($handle))) {
   -      if (!preg_match($options['nomask'], $filename) && $filename[0] != '.') {
   +      if ($filename[0] != '.'
   +          && !(isset($options['nomask']) && preg_match($options['nomask'], $filename))
   +          && !(!empty($default_nomask) && preg_match($default_nomask, $filename))
   +          ) {
   

   This looks more complicated than necessary. Is there any reason not to just set $options['nomask'] in the top code, then not change the bottom code at all? The whole $default_nomask variable really doesn't seem like it needs to exist.

   (This suggestion applies to Drupal 8 too, I suppose.)

Good points.
We are using this patch on production on around 16 sites running on PHP 5.6 and the patch works very well.

1. It was assumed that by shipping it with defaults would instantly give the performance benefits to everyone without doing anything. Not all the users follow CHANGELOG to see what is new and what manual steps they need to do. Talking about this, I think we should update the changelog too.

2. Definitely missed that

3. I tried to keep the patch as close as possible to the D8 version, we could change the logic if necessary, but that would require further testings.

1) Changed the default to an empty array.

2) I took a stab at this but I'm not sure where $value was coming from in the previous patch. +1 to making this bit of code easier to understand.

3) Included an attempt to rework this section without $default_nomask to make it simpler.

Patch in #38 applies cleanly to 7.59, but I'd love some help with the failing tests.

I wasn't noticing anything odd for a year until I installed Lando which is a bit slower on Mac because of virtualisation. admin/config page was loading for about 20-30 seconds. Blamed Lando and Docker and almost gave out.
Luckily first I discovered that there was 3 modules that where not available in file system anymore and this is known to clog the system. For this I found this fix:

$files = db_select('system', 's')
  ->fields('s', array('filename', 'status'))
  ->execute()
  ->fetchAll();
foreach($files as $obj) {
  if(!file_exists($obj->filename)) {
    drupal_set_message(t('@fn is missing.  Status is @stat.', array('@fn' => $obj->filename, '@stat' => $obj->status)), 'error');
    // Uncomment the following 2 lines to delete missing files from the system table.  Backup your DB first!
    //db_delete('system')->condition('filename', $obj->filename, '=')->execute();
    //drupal_set_message(t('@fn deleted from system table', array('@fn' => $obj->filename)));
  }
}

I got the speed down to about 10 seconds which was still slow. Did some profiling and found the file_scan_directory() function. It was listing thousands of folders inside one of my custom module I built a year ago - it was the node_modules folder. When I removed that then site/admin folder ran about 2 seconds in Lando. Yay!

My 2 cents for this issue would be to treat node_modules (and bower_components) as importantly as . and CVS folders these days. CVS is probably not as popular even as node_modules is. So why not just hard-code it next to the other folders?

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

I mean nobody would want to have possibility to remove these. 99% would miss the configuration option if it was optional.

drupal@guusvandewal.nl: Instead of passive-aggressively complaining that the problem hasn't been fixed yet, maybe see if you can help resolve the problem, or fund time for someone else to fix it?

We have been using patch #31 for a year and it works fine, I know that there were some comments and new patches after it, but I did not have time to work on this issue.
All I know is that the patch #31 works for us.

Here is a patch that simply makes the nomask regexp a variable.
This doesn't change any functionality and can at least be overridden.

What about adding |node_modules|bower_components to the defaults?

At European Commission we are using patch #31 since a long time without any single issue.

I'd like to push it forward, is everyone ok with this ?

Refactoring the patch using.

Adressing remarks from @David Rothstein and tests should be green now.

Updating the patch.

Convert new array syntax to old array syntax.

I uploaded the wrong patch, reuploading the proper one.

Fix patch and tests.

Update patch, add forgotten staged parts.

Can you please upload the interdiff between your patch and #31

1. +++ b/includes/file.inc
   @@ -2130,9 +2130,30 @@ function file_download_access($uri) {
   +    $ignore_directories = array_map(
   +      function ($value) {
   +        return preg_quote($value, '/');
   +      },
   +      variable_get('file_scan_ignore_directories', array())
   +    );
   

   I think a simple loop is easier to read than using array_map here.

2. +++ b/includes/file.inc
   @@ -2130,9 +2130,30 @@ function file_download_access($uri) {
   +    if ($ignore_directories !== array()) {
   +      $nomask = '/^(\.\.?)|CVS|' . implode('|', $ignore_directories) . '$/';
   +    }
   

   if (!empty($ignore_directories)

   makes more sense here ...

3. +++ b/sites/default/default.settings.php
   @@ -645,3 +645,17 @@ $conf['404_fast_html'] = '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
   +$conf['file_scan_ignore_directories'] = array(
   +  'node_modules',
   +  'bower_components',
   +);
   

   This will need a change record.

   Also:

   How was this solved in D8? Is it configurable there?

A bit of code tidying and the interdiff.

@Fabianx via settings.php https://www.drupal.org/node/2329453

Updated patch based on @fabianx's feedback.

Interdiff with #31 has been uploaded as well.

In Drupal 8, this has been fixed 2 years ago in the same way (see #2329453: Ignore front end vendor folders to improve directory search performance).

This will need a change record for the new variable in default.settings.php (the one from D8 can likely be copied).

It will also need special attention in release notes

Updated patch, remove "out-of-scope" improvements I introduced and leave only the strict necessary.

Interdiff with #31 included.

Working on the Change Record now.

Change record: https://www.drupal.org/node/3024333

Please review it and provides some feedback.

Looks good now

Updating credits based on this thread and from #2329453: Ignore front end vendor folders to improve directory search performance.

Fixed!!!

Thanks everybody !!!

C'mon guys, you can't use short array syntax in Drupal 7. Whatever happened to testing things before committing?

See https://www.drupal.org/pift-ci-job/1166553

This commit broke ALL PHP 5.3 tests for both core and contrib.

https://www.drupal.org/core/issue-priority

Critical bugs include those that:

• Cause tests to fail in HEAD on the automated testing platform for any supported environment (including random failures), since this blocks all other work.

PHP 5.3 isn't supported anymore.

Update: My comment is based on what Dries wrote about PHP 5. I note that the PHP requirements page doesn't match this.

In any case, it's not worth breaking compatibility over short array syntax.

@Liam Morland: That's a ridiculous and inaccurate claim. Drupal 7 supports PHP >= 5.2.4 and PHP 5.3 tests are a standard part of core and contributed testing. If it weren't for this one little [] instead of array() introduced by the above patch, all these tests would still be running.

Instead of adding fuel to the flames... how about an RTBC?

FTR, 7.62 hasn't been tagged yet... so there's still time.

Great catch @TR.

+1 to commit follow-up fix

Fixed.

#2581193: [Drupal 7] Require PHP 5.3

This commit seems change the default.settings.php permission from 644 to 755. Anyone know why that is the case?

I don't see anything in this commit that would touch default.settings.php at all. It must be something else.

https://www.drupal.org/project/drupal/issues/2482549#comment-12917581
This one did it.

Thanks, Pol. #3035772: [Regression] Fix default.settings.php permission fixed the file permissions.