Problem

  1. Drupal\system\Tests\System\ScriptTest uses exec('core/scripts/password-hash.sh), which assumes that #!/bin/env php is a thing. Executing this test on Windows starts two instances of my file/code editor with the specified files.
  2. Due to the separate PHP process, the scripts are executed against the parent site.
CommentFileSizeAuthor
#8 test.scripts.8.patch6.56 KBsun
#5 interdiff.txt1.89 KBsun
#5 test.scripts.5.patch7.07 KBsun
#1 test.scripts.1.patch7.19 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new test.scripts.1.patch7.19 KB

Attached patch fixes the mess.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/System/ScriptTest.php
--- a/core/scripts/password-hash.sh
+++ b/core/scripts/password-hash.php

+++ b/core/scripts/password-hash.php
+++ b/core/scripts/password-hash.php
@@ -1,22 +1,26 @@

@@ -1,22 +1,26 @@
-#!/usr/bin/env php
 <?php

Not sure we have to remove this, with it, you can execute it both with php file and ./file.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The #!/usr/bin/env php approach and false-exposure of a PHP script as a shell script has caused us more problems than it resolves. (There are a bunch of issues in the queue.)

We should stop doing that. It's just reasonable and not too much to ask to execute a PHP script via $ php scriptname.php

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@sun re #3 is this in scope and necessary to fix this?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new test.scripts.5.patch7.07 KB
new interdiff.txt1.89 KB

Renamed scripts back to .sh.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

5: test.scripts.5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 5: test.scripts.5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new test.scripts.8.patch6.56 KB
ParisLiakos’s picture

ParisLiakos commented 
+++ b/core/scripts/password-hash.sh
@@ -37,55 +42,31 @@
-$passwords = array();
...
-// Parse invocation arguments.
-while ($param = array_shift($_SERVER['argv'])) {
-  switch ($param) {
-    case '--root':
-      // Change the working directory.
-      $path = array_shift($_SERVER['argv']);
-      if (is_dir($path)) {
-        chdir($path);
-      }

i do not understand why we have to remove this functionality?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@ParisLiakos: It's no longer needed. autoload.php and bootstrap.inc are included in the same way as in the other scripts.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

ah, correct, thanks!

Before this patch:

Drupal\system\Tests\System\ScriptTest                          0 passes   4 fails

After this patch

Drupal\system\Tests\System\ScriptTest                          4 passes

Code looks coode, thus RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: test.scripts.8.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review

8: test.scripts.8.patch queued for re-testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

testbot fluke

  • Commit ed6861b on 8.x by Dries: 
    Issue #2248985 by sun: ScriptTest fails on Windows, runs against parent...
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good but couldn't test it on Windows. Committed to 8.x because it ships with tests. Thanks.

Status: Fixed » Closed (fixed)

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