Symlinking a module breaks HookCollectorPass

This test is not actually working at the moment so I'm pulling it out for a cleaner commit:

<?php

declare(strict_types=1);

namespace Drupal\KernelTests\Core\Hook;

use Drupal\Core\Hook\HookCollectorPass;
use Drupal\KernelTests\KernelTestBase;
use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
 * @coversDefaultClass \Drupal\Core\Hook\HookCollectorPass
 * @group Hook
 */
class HookCollectorPassTest extends KernelTestBase {

  protected function setUpFilesystem(): void {
    // VFS does not and can not support symlinks.
  }

  public function testSymlink(): void {
    mkdir($this->siteDirectory);
    symlink(realpath("core/modules/user/config"), "$this->siteDirectory/config");
    $container = new ContainerBuilder();
    $module_filenames = [
      'test' => ['pathname' => "$this->siteDirectory/test.info.yml"],
    ];
    $container->setParameter('container.modules', $module_filenames);
    (new HookCollectorPass())->process($container);
    unlink("$this->siteDirectory/config");
    rmdir($this->siteDirectory);
  }

}

Ah I see you've added and removed a test, bring on gitlab issues where there's only one place to track an issue 🫠

Yes we removed the test because the symlink part is dependent on the env, so in order to keep this fix clean for now it's just the fix.

If you can review the test attached in 8 and provide advice we can readd it.

This issue is specifically to how gitlab runs tests, it's treating the symlinks as directories.

Can someone update the issue summary with the reasons this is a critical bug?

This issue is caused not by gitlab per se. It's caused by https://www.drupal.org/project/gitlab_templates → and how https://git.drupalcode.org/project/gitlab_templates/-/blob/main/scripts/... works.

I updated the IS but I used this criteria:

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

Given contrib tests are broken I think we should commit the hotfix here, it's not as if we're going to silently break this - was noticed within hours.

I think I might have a test.

Ok there is a test.

I have a positive test coming soon.

Ok there is now a test confirming a notice is not thrown when a symlink is encountered and a positive confirmation that hooks inside the symlinks are picked up.

I removed the negative test per @alexpott and @elc's comments. There is a positive assertion now too to confirm it read the hooks.

My only concern is the symlink failure was silent so ensuring that notice isn't appearing is probably worth it, but we have the history if we want to add it back.

For credit purposes other than the people chiming in on this issue.

@chi and @claudiu.cristea both alerted me to the issue here: 📌 OOP hooks using event dispatcher Needs review and helped investigate in slack.

@fjgarlin pointed out how to patch core in a contrib test so we could verify the fix there and @hestenet helped loop in @fjgarlin.

@elc also provided a review, and I used a module he maintains as the test ground.

@ghostofdrupalpast identified the root cause and fix.

I pushed up a commit to ensure that the test fails if symlinks are not followed and triggered the test only change.

alexpott → credited chi → .

alexpott → credited claudiu.cristea → .

alexpott → credited elc → .

alexpott → credited fjgarlin → .

Test-only change fails as expected... https://git.drupalcode.org/project/drupal/-/jobs/3120262

Just fixing PHPStan baseline.

And adding issue credit as per #23

I've fixed up the test which has given me a good chance to review the low level code fix here and the surrounding code. This makes sense and is similar to the code in \Drupal\Core\Extension\ExtensionDiscovery::scanDirectory() which is doing something along the same line. I've checked the other options used (\FilesystemIterator::CURRENT_AS_SELF, \RecursiveIteratorIterator::LEAVES_ONLY and \RecursiveIteratorIterator::CATCH_GET_CHILD) there and I do not think they are not needed in this code.

We probably want to get a cleanup phase in the test as well since this is not in vfs:

foreach (scandir($this->siteDirectory) as $item) {
      $target = "$this->siteDirectory/$item";
      if (is_link($target)) {
        unlink($target);
      }
    }

I removed it locally when I was confirming symlink creation, but didn't add it back.

Needs work for #31, that's a good point.

@nicxvan @catch let's open a follow-up to do that. We should fix all KernelTests that use proper files and doing that reveals quite a nasty bug in \Drupal\Core\File\FileSystem::deleteRecursive!

Discussed the clean-up with @alexpott and @nicxvan briefly in slack and a follow-up is incoming. Going to go ahead and commit here.

Committed/pushed to 11.x, thanks!

🐛 Cleanup real files in KernelTestBase tests Active is the follow-up.

catch → closed merge request !9901

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