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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Active » Needs review
FileSize
7.19 KB

Attached patch fixes the mess.

Berdir’s picture

+++ 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

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

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

sun’s picture

FileSize
7.07 KB
1.89 KB

Renamed scripts back to .sh.

sun’s picture

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

Status: Needs work » Needs review
FileSize
6.56 KB
ParisLiakos’s picture

+++ 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

@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

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

Status: Needs work » Needs review

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

ParisLiakos’s picture

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

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.