Problem/Motivation

When theme has node_modules directory scanning goes long and shows false positives

Steps to reproduce

 STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 0    The spaceless tag in                                        
                    "themes/custom/custom_theme/node_modules/twig-loader/test/fixtures/in
                    clude/template.html.twig" at line 19 is deprecated since    
                    Twig 2.7, use the "spaceless" filter with the "apply" tag   
                    instead. See https://drupal.org/node/3071078. 

Proposed resolution

Add node_modules to exclude like https://github.com/mglaman/drupal-check/pull/184

Remaining tasks

patch, review. commit

User interface changes

no

API changes

no

Data model changes

no

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Status: Active » Needs review
FileSize
303 bytes

Simple patch, not sure it could be covered with tests

Patch applies to 8.x-2.x as well

andypost’s picture

Issue summary: View changes

Added example to steps to reproduce with custom theme

andypost’s picture

Version: 8.x-3.x-dev » 8.x-2.x-dev
andypost’s picture

Status: Needs review » Needs work

It needs work because twig provides no way to exclude directories from scan
Filed
- https://github.com/twigphp/Twig/issues/3382
- https://github.com/zimmo-be/twig-loader/pull/59

andypost’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
1.48 KB

We could use Finder to exclude core provided directories

Status: Needs review » Needs work

The last submitted patch, 6: 3162997-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.63 KB
3.78 KB

There's second place where templates are discovered, maybe it makes sense to create separate helper method (could be static)

uzlov’s picture

cool
one note

+              if ($name[0] == '.') {
+                return FALSE;
+              }
+              return !in_array($name, $exclude, TRUE);

can be more simple
just return ...

andypost’s picture

+++ b/src/DeprecationAnalyzer.php
@@ -442,7 +444,25 @@ final class DeprecationAnalyzer {
+        new \RecursiveCallbackFilterIterator(
...
+                return FALSE;
...
+              return !in_array($name, $exclude, TRUE);

The https://www.php.net/manual/en/class.recursivecallbackfilteriterator.php expecting the boolean as result from callback

uzlov’s picture

I mean, just
return $name[0] == '.' ? FALSE : !in_array($name, $exclude, TRUE);

andypost’s picture

Status: Needs review » Needs work

Great hint, thanks!

Moreover I think better to add special TwigTemplateIterator to encapsulate it

andypost’s picture

FileSize
3.28 KB

Separate iterator approach, still needs test

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Adding the needs tests tag. I think adding a node_modules dir with a twig file using something that would become an error in one of the passing modules would be good to prove this :)

+++ b/src/TwigRecursiveIterator.php
@@ -0,0 +1,33 @@
+  public function __construct(string $directory) {
+    $exclude = Settings::get('file_scan_ignore_directories', []);
+    parent::__construct(new \RecursiveCallbackFilterIterator(
+      new \RecursiveDirectoryIterator($directory, \RecursiveDirectoryIterator::SKIP_DOTS),
+      function ($current) use ($exclude) {
+        $name = $current->getFilename();
+        // RecursiveDirectoryIterator::SKIP_DOTS only skips '.' and '..', but
+        // not hidden directories (like '.git').
+        return $name[0] !== '.' &&
+          (($current->isDir() && !in_array($name, $exclude, TRUE)) ||
+            ($current->isFile() && substr($name, -10) === '.html.twig'));
+      }
+    ), \RecursiveIteratorIterator::LEAVES_ONLY);
+  }

Hm, which part of this will exclude node_modules or bower_components for that matter?

If TemplateDirIterator already does so, why do we need this one specifically?

andypost’s picture

($current->isDir() && !in_array($name, $exclude, TRUE)) this part, the same we do in core

andypost’s picture

Assigned: Unassigned » andypost

++ to add tests

Gábor Hojtsy’s picture

@andypost: are you still planning to work on this? Thanks!

andypost’s picture

Assigned: andypost » Unassigned

Yes, I do, but not this week(

Gábor Hojtsy’s picture

Would love to add this with tests, but I don't have capacity to add tests unfortunately.

vacho’s picture

FileSize
3.22 KB

I only rebase by now the patch #13

vacho’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
Gábor Hojtsy’s picture

Sent the 3.x for a retest on 8.9 for now as the patch applies locally.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.1 KB
896 bytes

Attempted a simple test for this.

Gábor Hojtsy’s picture

FileSize
896 bytes

Actually the interdiff was supposed to be a test only patch that should fail.

Status: Needs review » Needs work

The last submitted patch, 24: 3162997-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • 1f2e226 committed on 8.x-3.x
    Issue #3162997 by andypost, Gábor Hojtsy, vacho, uzlov: Exclude...
Gábor Hojtsy’s picture

Status: Needs work » Fixed

That proved that the twig file lookup and the phpstan lookup both exclude node_modules now. Let's get this in! Thanks a lot!

Status: Fixed » Closed (fixed)

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