Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
\Drupal\Core\File\FileSystem::deleteRecursive() can result in data loss due to symlinks to directories outside of the directory being deleted. If the link is to a file required for the running of your site this could result in data loss or the site breaking.
Additionally, some kernel tests override \Drupal\KernelTests\KernelTestBase::setUpFilesystem() to use a real file system for testing things that cannot be tested on vfs. For example any test that extends \Drupal\KernelTests\Core\File\FileTestBase. These tests do not currently clean-up after themselves. This was noted in #3482283: Symlinking a module breaks HookCollectorPass
Steps to reproduce
Run the new test added in the MR.
Run \Drupal\KernelTests\Core\Archiver\ZipTest and see the directory left in sites/simpletest
Proposed resolution
Fix deleteRecursive to work with symlinks correctly.
Add functionality to KernelTestBase to do the cleanup if necessary
Remaining tasks
User interface changes
N/a
Introduced terminology
N/a
API changes
N/a
Data model changes
N/a
Release notes snippet
N/a
Issue fork drupal-3482449
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3482449-cleanup-real-files
changes, plain diff MR !9905
Comments
Comment #2
nicxvan commentedDo we want a flag to allow them to stay for local debugging if you need to see the actual files created for some reason?
And maybe a way to confirm that cleanup happened so future tests have to clean up after themselves.
Comment #4
alexpott@nicxvan no - we don't offer that in WebDriverTestBase which does the same thing. You can always preserve the environment by adding an exit() to the test.
FWIW the fix to \Drupal\Core\File\FileSystem::deleteRecursive() almost makes me want to make this issue about that and not KTB and tidying up. This feels like a serious data destruction bug - we should not be recursing into links and removing files...
Comment #5
nicxvan commentedexit() makes sense, my only thought was sometimes you might need to see more broadly, but that is probably super edge case and not worth the effort.
Comment #6
alexpottChanging the focus of the issue to be the data loss bug discovered here by trying to cleanup the new \Drupal\KernelTests\Core\Hook\HookCollectorPassTest::testSymlink() test.
Comment #7
alexpottI extended test coverage to include links to files outside of the directory being deleted. This is not currently broken but it feels good to have a test for.
Comment #8
alexpottSo added some test coverage of the expanded delete functionality. Which opens up more worms because of the way that streams work with links and resolve to the real file or directory and not link. Which imo results in very odd behaviour when you call delete.
Comment #9
nicxvan commentedI took a look at this again, it seems pretty straightforward.
The only question I have is this worth a comment mentioning the risk to consider if this gets changed in the future?
Unless you think the comment for the tests is enough:
Tests symlinks in directories do not result in unexpected deletions.It might be helpful for people using this as example code for themselves to see what to consider.
Comment #10
alexpott@nicxvan I think the additional test coverage added here is fine.
Comment #12
smustgrave commentedRebased because it was 190 commits back.
Funny I got 6 random errors bur re-running twice fixed those
All the new tests appear to fail in the test-only job https://git.drupalcode.org/issue/drupal-3482449/-/jobs/3688988
Dont't see any open threads believe this is good.
Comment #15
catchCommitted/pushed to 11.x and cherry-picked to 10.5.x, thanks!
Comment #18
cilefen commentedThere is a regression reported: #3559132: FileSystem::deleteRecursive() leaves files/directories when realpath() returns FALSE.