Remove dependency on symfony/finder

symfony/finder actually is already required by Drupal core since #3031379: Add a new test type to do real update testing and 8.8.0 … so nothing to do there! 🥳

#7: D'oh, great point — I missed that! 🙈 Thanks! 🙏

namespace Symfony\Component\DependencyInjection\Loader;

[…]

class DirectoryLoader extends FileLoader

+

namespace Symfony\Component\DependencyInjection\Loader;

[…]

use Symfony\Component\Config\Loader\FileLoader as BaseFileLoader;

[…]

abstract class FileLoader extends BaseFileLoader

+

namespace Symfony\Component\Config\Loader;

[…]

/**
 * FileLoader is the abstract class used by all built-in loaders that are file based.
 *
 * @author Fabien Potencier <fabien@symfony.com>
 */
abstract class FileLoader extends Loader

→ I don't see how we can avoid using symfony/config if we don't want to make \Drupal\package_manager\PackageManagerServiceProvider::register() a lot more complicated.

https://symfony.com/doc/current/components/config.html says:

The Config component provides several classes to help you find, load, combine, fill and validate configuration values of any kind, whatever their source may be (YAML, XML, INI files, or for instance a database).

… which sounds like a reasonable component to include in Drupal core.

symfony/finder is used in:

• \Drupal\package_manager\PathExcluder\GitExcluder
• \Drupal\package_manager\PathExcluder\NodeModulesExcluder
• \Drupal\package_manager\Validator\DuplicateInfoFileValidator
• … as well as in a number of tests

I'm tempted to say that that code clarity is worth it.

But Drupal also has a drupal/core-filesystem component, which has a pretty capable \Drupal\Component\FileSystem\RegexDirectoryIterator. Trying to convert to that…

Been fighting \RecursiveFilterIterator and friends, getting there. Should be able to wrap this up tomorrow.

If you agree with this direction, we'll need to add #3326246: Add symfony/config to core's dependencies for package_manager to #3319030: Drupal Core Roadmap for Package Manager and Update Manager.

Merged in the upstream changes from the past ~month.

Explicitly assigning to @tedbow to (finally 😅) get feedback.

I still cannot reproduce the DrupalCI-only failures locally. Here's hoping that was an ephemeral problem on the DrupalCI infrastructure … 🤞

It clearly isn't ephemeral. @tedbow: can you reproduce the failures locally?

Thanks, great research, @tedbow! 👍

After discussing with @tedbow, I have some ideas on how to move forward here now — there likely is a lot of additional directories on DrupalCI that we don't have locally, but we don't know where yet 🫣

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

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!

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 #3332256: package_manager_bypass should *minimally* bypass php-tuf/composer-stager, not maximally and #3335908: The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed (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    
…

⚠️ While #49 matches HEAD's current behavior, after #3319507: Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly lands, we will be accepting most symlinks. Which means validators should also follow them while validating. Captured at #3319507-26: Add symlink support to Composer Stager 2.0, require that version, and simplify UX & tests accordingly.

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! 🥳

#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 😊

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

#61: I did document that as part of working on this issue: see #11, and #3326246: Add symfony/config to core's dependencies for package_manager. 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.