Remove dependency on symfony/finder

Now that 🐛 phpcs stopped working since the switch to testing on Drupal 10.0.x by default Fixed is finally done, this is the next most important issue! Will continue this tomorrow.

Drupal CI behaves weirdly , does not run tests according to drupalci.yml , hence the retests.

Test times

New PR
kernel - 6:24 mins
functional - 13 mins
build - 14 mins

baseline
kernel - 7.24
functional - 5.42
build - 12 mins

We can clearly see that functional tests are running slower , so i thought the only thing that functional tests are using that we changed is package_manager/tests/src/Traits/FixtureUtilityTrait.php but even without the changes in it they still seem to run slower.

Performance test results

The old finder and the RecursiveDirectoryIterator are definitely looking at different files and the RecursiveDirectoryIterator is making more passes on the same files (indicated by the numbers in front of the discovered file names)

From the latest test i think i can conclude 2 things

1. There are no weird subdirectory inside subdirectory folders from a previous run or any old undeleted files
2. The RecursiveDirectoryIterator is the only one that see's the weird structure.

I did try to create a symlink locally but was unable to recreate the same pattern of paths.

It doesn't need to be me who works on this, so if somebody wants to dig into this before I get to it, please do!

When I debugged this I get ~7500 from withOutFinder() and ~250 files from withFinder().

Test failing with on self::assertSame($txt_files_finder, $txt_file_no_finder); line in \Drupal\Tests\automatic_updates\Unit\PerformanceTest::testPerformance(). The results are identical still the test is failing.

Need help with the error

Reproduced the infamous recursive Omkar locally:

…
+    '/Users/wim.leers/core/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/sites/simpletest/36713557/ygxJCv2dGVN5BPYVCRvz/modules/example/.git/ignore.txt' => 1
+    '/Users/wim.leers/core/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/sites/simpletest/18800034/files/config_gKQI85yVN-z5lKiIDAsy5GJSfwwYfJeBfF3z64BkA6qwCgXD-pg4m_mECxzSHxcTvdV_RwLHCQ/sync/README.txt' => 1
+    '/Users/wim.leers/core/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/sites/simpletest/18067332/bP1bRMh0Cq4TfBGnoQUr/core/node_modules/ignore.txt' => 1
+    '/Users/wim.leers/core/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/omkar/sites/simpletest/18067332/bP1bRMh0Cq4TfBGnoQUr/sites/default/stage.txt' => 1
…

→ I'll continue here.

Turns out that the reason the 3321933-performance-test is actually very simple:

• withFinder() never calls \Symfony\Component\Finder\Finder::followLinks(), which is why it is ignoring symlinks entirely
• withOutFinder() specifies $flags |= \FilesystemIterator::FOLLOW_SYMLINKS;, which causes it to do the opposite

⇒ the recursive Omkar mystery is solved: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/64...

That puts us at:

+    '/Users/wim.leers/core/omkar/core/.deprecation-ignore.txt' => 1
     '/Users/wim.leers/core/omkar/core/INSTALL.sqlite.txt' => 1
     '/Users/wim.leers/core/omkar/core/INSTALL.pgsql.txt' => 1
     '/Users/wim.leers/core/omkar/core/lib/Drupal/Core/README.txt' => 1
@@ @@
     '/Users/wim.leers/core/omkar/sites/simpletest/29115714/fXQhTRGlQ1k5Az4B6AYc/sites/example.com/files/ignore.txt' => 1
     '/Users/wim.leers/core/omkar/sites/simpletest/29115714/fXQhTRGlQ1k5Az4B6AYc/sites/simpletest/ignore.txt' => 1
     '/Users/wim.leers/core/omkar/sites/simpletest/29115714/fXQhTRGlQ1k5Az4B6AYc/private/ignore.txt' => 1
+    '/Users/wim.leers/core/omkar/sites/simpletest/29115714/fXQhTRGlQ1k5Az4B6AYc/.git/ignore.txt' => 1
     '/Users/wim.leers/core/omkar/sites/simpletest/29115714/fXQhTRGlQ1k5Az4B6AYc/modules/example/node_modules/ignore.txt' => 1
+    '/Users/wim.leers/core/omkar/sites/simpletest/29115714/fXQhTRGlQ1k5Az4B6AYc/modules/example/.git/ignore.txt' => 1
     '/Users/wim.leers/core/omkar/sites/simpletest/11853257/files/config_MnP16Yj8pfbM5wIkoIceZlpcxP5K1FivVn1ZzPJ9iY0ZkGih2Fc-hM2q

→ better, but still not perfect.

Reason for that: we're again not yet matching Finder's behavior, which has this:

    public function __construct()
    {
        $this->ignore = static::IGNORE_VCS_FILES | static::IGNORE_DOT_FILES;
    }

… while we have nothing like that at all in our logic in PerformanceTest.

Now tests pass:

PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing /Users/wim.leers/core/modules/contrib/automatic_updates/tests/src/Unit
....................                                              20 / 20 (100%)

Time: 00:31.553, Memory: 10.00 MB

OK (20 tests, 40 assertions)
Process finished with exit code 0

So it's faster 100% of the time. Adding

var_dump(['with finder' => $with_finder, 'without finder' => $without_finder]);

makes that more clear:

Rarray(2) {
  ["with finder"]=>
  float(0.9062411785125732)
  ["without finder"]=>
  float(0.6564769744873047)
}
Rarray(2) {
  ["with finder"]=>
  float(0.9076869487762451)
  ["without finder"]=>
  float(0.6771509647369385)
}
Rarray(2) {
  ["with finder"]=>
  float(0.9273748397827148)
  ["without finder"]=>
  float(0.6514010429382324)
}
Rarray(2) {
  ["with finder"]=>
  float(0.9032847881317139)
  ["without finder"]=>
  float(0.6541171073913574)
}
<snip>

I bet that the root cause for the actual MR (https://git.drupalcode.org/project/automatic_updates/-/merge_requests/59...) was similar: none of the code in package_manager is calling Finder::followLinks()!

New MR opened … waiting for it to fail before pushing the commit that will remove \FilesystemIterator::FOLLOW_SYMLINKS 🤓

Looks like \FilesystemIterator::CURRENT_AS_SELF is causing phpstan confusion? 😳 😬 Trying more…

Well well well … looks like this even works just like that, by just literally porting the MR I had created months ago against 8.x-2.x to 3.0.x (and fixing the PHPStan violations which even then were not running anymore apparently) … 😳

I bet:

• it's because some significant test infrastructure changes happened since #18 (January 4, almost 2 months ago) — most notably 📌 package_manager_bypass should *minimally* bypass php-tuf/composer-stager, not maximally Fixed and 🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed (which specifically touches on symlink-related things).
• those have removed a significant number of symlinks that the file system iterating/filtering was previously choking on.

But! It's still slower! The functional tests no longer time out like in January, but they run excruciatingly slow. So pushed a commit just now to stop following symlinks, just like the validators were doing previously.

Hah! ec30ba81 even finished before d2c2aac2 even despite it starting tests roughly an hour later… 😅 In fact, it's currently still going:

…
00:51:49.032 Drupal\Tests\automatic_updates_extensions\Functional\PreUpda   3 passes                                      
01:21:38.843 Drupal\Tests\automatic_updates_extensions\Functional\UpdateE   3 passes                                      
01:21:40.820 Drupal\Tests\automatic_updates_extensions\Functional\Display   2 passes                                      
01:21:41.494 Drupal\Tests\automatic_updates\Functional\TableLooksCorrectT   2 passes    
…

Wim Leers → closed merge request !697

Wim Leers → closed merge request !647

Wim Leers → closed merge request !593

⚠️ While #49 matches HEAD's current behavior, after 📌 Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly Fixed lands, we will be accepting most symlinks. Which means validators should also follow them while validating. Captured at #3319507-26: [upstream] Support more symlinks, document why certain symlinks are not supported → .

CorePackageManifestTest and FixtureUtilityTrait will not be affected.

Posted informative comments on the open MR, closed all other MRs. This is now hard-blocked on review, at last! 🥳

This looks really good. I think it could maybe use a little consolidation and documentation but I have no objections otherwise.

#3305568: Create a validator that detects duplicate info.yml files in the stage on apply → introduced DuplicateInfoFileValidator. It is what introduced ignoreUnreadableDirs() without documenting why. So that leaves some lack of clarity about how we should support this.

So I propose to instead just reuse core's ExtensionDiscovery 😊

A question, a request to revert something that didn't need to change, and a couple of nits. Then this, in my opinion, is ready and I will be glad to see it land!

All feedback addressed! 👍 Some really good catches in there! 👏 Thanks for the thorough review 😊

Re-titling, because the final MR does not document why we need symfony/config. That's okay, though.

Glad to see this alpha blocker bite the dust!

Re-titling, because the final MR does not document why we need symfony/config.

Here's a follow-up for that: 📌 Remove dependency on symfony/config Fixed

#61: I did document that as part of working on this issue: see #11, and 📌 Add symfony/config to core's dependencies for package_manager Closed: duplicate . This is all stated in the issue summary. Nowhere in Drupal core is it documented in code why a certain composer package is being depended upon, so doing that here would've been out of scope.

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