Use the PHP_BINARY constant in addition to --php in run-tests.sh

This has been implemented in #2624926: Refactor run-tests.sh for Console component. for Drupal 8. Maybe the effort should be consolidated to there? Unless we feel like there should be a incremental fix.

Yes, it seems maybe out of scope. Just a note for anyone fixing this: Unlike in the D7 issue where we must add a conditional for the constant in order to support PHP < 5.4, for D8 we can simply replace the conditional looking for the "_" environment variable with one looking for the constant. Are we interested in (EDITED) deprecating --php as useless?

Technically, having --php around allows you to use a different version of PHP to test against. So for example... if we have php 5.4 as our Global php, we could use php 5.6 to test against by defining --php

With that said, I can see the other side of it... if you want to use php 5.6... run the script in php 5.6... It really depends on how the CI is set up and if it actually uses this script and/or the --php argument when testing patches and commits.

True. And we can't just remove --php anyway in case anyone depends on it and it would break existing workflows. But we could toy with deprecating it. But why stress over it, because #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

@dawehner,
your comment is redundant to what I said in the second part of my comment.

Regardless, lets start with having --php in the patch... that will be a starting point and have a patch for those who need this.

The testbot uses --php to force the php version.

If it's not present, you'd want the same binary as the one you're running under, which isn't always what PHP_BINARY tells you. PHP doesn't set it from the currently-running binary, it sets it through some shell that doesn't see your .profile, so it could pick up the wrong one, depending.

That's why getenv('_') is present, too. We want that. :-)

This is also a problem for the simpletest UI runner: #2476139-17: Fix UI test fail in SimpleTestBrowserTest

+++ b/core/scripts/run-tests.sh
@@ -391,9 +391,9 @@ function simpletest_script_init() {
-  elseif ($php_env = getenv('_')) {
-    // '_' is an environment variable set by the shell. It contains the command
-    // that was executed.

Don't remove this.

I would like to see a proof of a PHP constant set in a .profile. If you have some documentation of how PHP_BINARY is set, please share.

In fact the _ variable doesn't exist on Windows and a few other Operating Systems without installing shell.

I would like to see a proof of a PHP constant set in a .profile.

I mean that if you have two PHP binaries, as you do with a MAMP setup, then you're going to set a path to the MAMP binary in your PATH env. This leads to a problem when PHP is executed by the server, because exec() doesn't look at *your* shell dot files, it looks at some shell somewhere's dot files. At least as near as I can tell. Under CLI PHP_BINARY seems to do the right thing since it's executing as the same user.

So I was mistaken about the CLI, having experimented with it a bit.

Elaborated on the original patch to make sure the same ini file that was used when calling the tests.

This also unveiled the fact that $php does not need to be escaped when building the shell command, as it is not being used like an argument there.

Should we use \Symfony\Component\Process\PhpExecutableFinder instead?

@droplet That is so very true. Let's get rid of this old custom crafted logic.

It also ensure that the php runtime is generated including parameters (such as the php ini file, etc.).

Did not interdiff because the patch is mostly all new.

And this is how you make sure this is working as expected:

https://ci.appveyor.com/project/david-garcia-garcia/drupalcore-windows-t...

Thanks @david_garcia! The patch looks good to me.

For the same reasons that in #2748883: Use the PHP_BINARY constant in addition to --php in run-tests.sh, tests not working by default in a platform that has > 80% of desktop market share make this major.

Without this fix, the testing script is unable to find the PHP executable on Windows.

+++ b/core/scripts/run-tests.sh
@@ -440,20 +441,14 @@ function simpletest_script_init() {
-  elseif ($php_env = getenv('_')) {
-    // '_' is an environment variable set by the shell. It contains the command
-    // that was executed.
-    $php = $php_env;
-  }

@Mile23 has said that we want this. Since @Mile23 helps maintain DrupalCI we probably should have a good reason to remove this. As far as I can see, this change is not actually required to fix the problem. Why not leave that in?

@alexpott, @Mile23 PhpExecutableFinder() has a more elaborate logic to detect the php runtime, and it makes sense to use an upstream tool to do this.

It's true that PhpExecutableFinder() does not consider getenv('_'). Looking at the implementation, PHP_BINARY is only used when running in cli, cli-server and phpdbg, plus it has additional checks. So it looks they added some logic to ensure that PHP_BINARY is only used when it is trully reliable.

If something is broken or no totally correct in PhpExecutableFinder() it would make more sense to fix that upstream, as PhpExecutableFinder() is used in other places of Drupal core.

@david_garcia but the use of _ is specific to run-tests so not appropriate for an upstream fix - because PhpExecutableFinder is designed to work regardless of SAPI where run-tests.sh is designed to work from the CLI where the _ environment variable will be set on linux.

Brought back evnget('_'). Don't really like it, but will do no harm.

Rerolled the patch.

RTBC again. This is useful for Windows.

1. +++ b/core/scripts/run-tests.sh
   @@ -446,15 +447,14 @@ function simpletest_script_init() {
   -  elseif ($sudo = getenv('SUDO_COMMAND')) {
   -    // 'SUDO_COMMAND' is an environment variable set by the sudo program.
   -    // Extract only the PHP interpreter, not the rest of the command.
   -    list($php) = explode(' ', $sudo, 2);
   -  }
   

   I really don't know why we're removing this. It was added in #311160: Enhance run_tests.sh by automatically using right php interpreter and it is not part of doing better detection. I'm all for better detection on windows but there's no need to make unnecessary changes to a script that is used in people's build processes. There's no comment as to why this is breaking anything. As far as I can see, it is not. What is fixing things is the use of Symfony's php executable finder.

2. +++ b/core/scripts/run-tests.sh
   @@ -446,15 +447,14 @@ function simpletest_script_init() {
   +      $php = $php_executable_finder->find();
   +      if ($php == FALSE) {
   +        simpletest_script_print_error('Unable to automatically determine the path to the PHP interpreter. Supply the --php command line argument.');
   +        simpletest_script_help();
   +        exit(SIMPLETEST_SCRIPT_EXIT_FAILURE);
   +      }
   

   The indentation is incorrect here.

Hmm. I think we should put SUDO_COMMAND after PhpExecutableFinder as the last attempt if we still want to keep it.

For example, if we must run with sudo on the previous command but PHP does not.

sudo echo 'START Testing' && php ./testing.sh
sudo -i env && php ./testing.sh

(so this is a bug acutally)

And I guess it explained why only run-tests.sh on scripts folder missing #!/usr/bin/env php. So people don't run:

sudo bash ./run-tests.sh

Fixed #30

+++ b/core/scripts/run-tests.sh
@@ -441,20 +442,24 @@ function simpletest_script_init() {
-  elseif ($php_env = getenv('_')) {
-    // '_' is an environment variable set by the shell. It contains the command
-    // that was executed.
-    $php = $php_env;
-  }
   elseif ($sudo = getenv('SUDO_COMMAND')) {
     // 'SUDO_COMMAND' is an environment variable set by the sudo program.
     // Extract only the PHP interpreter, not the rest of the command.
     list($php) = explode(' ', $sudo, 2);
   }
+  elseif ($php_env = getenv('_')) {
+    // '_' is an environment variable set by the shell. It contains the command
+    // that was executed.
+    $php = $php_env;
+  }

Let's not change this order.

Also @pk188 that's not quite what @droplet was suggesting. However I think doing the smallest possible change here is advisable and not worry too much about SUDO_COMMAND and whether it works.

Let's just use Symfony's thing in the else case and be done.

SKIPPED THE PATCH, SEE NEXT COMMENT

OK. Not well coded but it works.

I suggested dropping a @deprecated and @todo over there to remove it at some point. ( I didn't add to current patch )

_ and SUDO_COMMAND was a hack to cheating while we do not PHP_BINARY in the old day (Just introduced from PHP5.2 PHP5.4)

You know how phpunit does it?

SebastianBergmann\Environment\Runtime->getBinary() which uses both $_SERVER['_'] and PHP_BINARY.

Experimentation tells me that this runs into the problem of double escapeshellarg() in combination with run-tests.sh.

Therefore if we want to benefit from not having to maintain this hunk of code in the future, we should rename global $php to global $escaped_php, adjust simpletest_script_command() to acknowledge that it's already escaped, and rawk on.

Now, will it blend? Let's find out. I had a problem with lots of failing tests locally with unmodified run-tests.sh, so I'm not sure what will happen here.

If it explodes too much, please ignore. :-)

run-tests.sh already warns the user if phpunit is not available, but sebastianbergmann/environment isn't, strictly speaking, the same as phpunit. We might need to explicitly declare it as a dev dependency. Or not.

yowza, means if you use have multiple version of php installed you call different versions and run the quitesuite against different verions :( :(. The indeed is major.

Things at least in the simpletest.module code have changed quite a bit but for the better, specifically fixing the Windows bug in #22, just linux didn't get the fix. We can just use the same code that makes windows work to resolve the php executable for all platforms and that should take care of it.

I've no idea what's going on in run-tests though so... maybe this is all that's needed now since everything is getting switched to phpunit tests?

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

With simpletest being removed in D10 is this still relevant?
If so please update IS with the rescope.