Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.When running random test using TestDatabase class under Windows it gets stuck in an infinite loop because of the following line:
web/core/lib/Drupal/Core/Test/TestDatabase.php:118
if (@symlink(__FILE__, $this->getLockFile($lock_id)) === FALSE) {
$lock_id = NULL;
}
Can be seen here as well: https://github.com/drupal/drupal/commit/649e2e68fc1e745f2e8f37530a8264c3...
Under Windows, you cannot create symlinks so simply and since that part of the code is unable to create symlinks, it gets stuck in infinite loop.
Only in version 8.2.0-rc2.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 16-17-interdiff.txt | 693 bytes | alexpott |
| #17 | 2804675-2-17.patch | 4.19 KB | alexpott |
| #16 | 2804675-2-16.patch | 4.11 KB | alexpott |
| #10 | 2804675-10.patch | 665 bytes | hideaway |
Comments
Comment #2
hideaway CreditAttribution: hideaway commentedComment #3
cilefen CreditAttribution: cilefen commentedComment #4
xjmWe will need to include this in the release notes for the next release one way or another (whether it's to flag a regression or that this was fixed since RC2).
Comment #5
alexpottAccording to the PHP documentation it says:
(https://secure.php.net/manual/en/function.symlink.php)
@hideaway which version of Windows did this fail on?
Comment #6
alexpottI wonder if there the temp directory is not writable - @hideaway can you run this patch on your environment?
Comment #7
daffie CreditAttribution: daffie commentedThe solution looks good to me. By throwing an exception for there not being a working temporary directory you are not getting to the infinite loop. All the code in the patch is good enough to be committed. Normally I would give this patch RTBC, but I do not have a Windows system to test this patch. So a big +1 from me.
Comment #8
hideaway CreditAttribution: hideaway commentedHi there.
Patch in #6 did not help and I explain why. In Windows in order to create symlinks you need to have "SeCreateSymbolicLinkPrivilege" privilege, which by default is held only by admin. Developers in our office are using Windows 10 Home and they do not have this privilege (and I really did not find a way how to add this in Win 10 Home). And since they cannot run phpunit command with admin rights or run their IDE (PHPStorm) with admin rights to run the tests from there, their migration tests got stuck in an infinite loop. If I run phpunit command with Admin privilege, of course then symlink is created, however since they cannot do this themselves, I currently patched that line of code to use @copy instead, which at least works for us and our migration tests are not stuck anymore.
Comment #9
daffie CreditAttribution: daffie commented@hideaway: Could you post the patch or the code changes that you made.
Comment #10
hideaway CreditAttribution: hideaway commented@daffie, of course I can. It was most simple change I could do in order to solve the problem.
Comment #11
daffie CreditAttribution: daffie commented@hideway: Maybe a stupid question, but do the migration tests now run or are they skipped?
Comment #12
hideaway CreditAttribution: hideaway commentedThey definitely run now :) To check I did the following:
1. I checked I have no test* tables in db
2. I run single test migration (like MigrateNodeTest)
3. Exited the test before db cleanup
4. Can clearly see the test* tables with the data.
Comment #13
daffie CreditAttribution: daffie commentedWith the explanation of @hideaway am I confident enough to give this patch a RTBC status.
Comment #14
catchCopy isn't atomic, so this could re-introduce the previous race condition.
We could possibly do something with rename()?
Whatever we do, it's going to need an explanatory comment.
Comment #15
Fabianx CreditAttribution: Fabianx as a volunteer commentedrename() is the only other atomic filesystem operation.
Comment #16
alexpottHere's another idea that will fix windows and probably a few other tricky situations.
Comment #17
alexpottA better comment.
Comment #18
alexpottComment #19
Fabianx CreditAttribution: Fabianx as a volunteer commentedFirst I didn't like that, but now I think this is a good solution.
Especially for unit tests a concurrency of more than one is just very seldom needed.
I think the patch is fine for both 8.3.x and 8.2.x, but probably needs a small change record or add to the existing yet to be created change record.
Comment #22
catchI think that's a good, honest, limitation.
Added a change record: https://www.drupal.org/node/2806653
Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!