The run-tests.sh script tries to find your php binary by checking $_ENV. This is not safe. If you have variables_order set to "GPCS" in php.ini (which is common), it will be empty. This is what the getenv() function is for. Patch fixes.

Also, it returns the results of the tests with simpletest_script_format_result(), which prints html special characters as if the output is html. Patch also fixes this by capturing the output and running it through htmlspecialchars_decode() so it's less ugly.

CommentFileSizeAuthor
#12 drupal-1477110-12.patch998 bytestim.plunkett
#3 un-f-runtests-script-3.patch1018 bytesmsonnabaum
un-f-runtests-script.patch1.39 KBmsonnabaum

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I'd RTBC the first hunk, but I'm not sure about the second one (which requires manual testing for various environments and scenarios [e.g., testbots]).

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

I have been meaning to do the first hunk forever. For some reason, the default PHP production configuration doesn't include E in variables_order (which seems slightly retarded, but that's another issue).

The second one looks like a hack: simpletest_script_format_result() is local to the script runner, so we could likely fix it not to return HTML in some cases (what are those? verbose results?)

msonnabaum’s picture

msonnabaum commented
Title: Make run-test.sh saner » Fix php path detection in run-tests.sh
Status: Needs work » Needs review
StatusFileSize
new un-f-runtests-script-3.patch1018 bytes

Ok, here's just the env fix. The output fix should probably have it's own issue.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

This looks backportable to D7, to me. Is it?

msonnabaum’s picture

msonnabaum commented

Totally backportable.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs backport to D7
dries’s picture

dries commented
Version: 8.x-dev » 7.x-dev

Committed to 8.x. Moving to 7.x. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 7.x-dev » 8.x-dev

This did not get pushed yet, setting back.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 8.x-dev » 7.x-dev

http://drupalcode.org/project/drupal.git/commit/4b6355f

msonnabaum’s picture

msonnabaum commented

Same patch should apply fine for D7 with -p2.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new drupal-1477110-12.patch998 bytes

Rerolled.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/scripts/run-tests.shundefined
@@ -253,14 +253,14 @@ function simpletest_script_init($server_software) {
-  elseif (!empty($_ENV['_'])) {
+  elseif ($php_env = getenv('_')) {

Does it look to anyone else like the little face is sadder than it used to be?

(RTBC)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

great thanks!

committed and pushed to 7.x.

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