Rename run-tests.sh to run-tests.php

I agree, but that is an issue for core and 7.x-2.x which I develop outside of core and patch into core.

Why can't it have the shebang? All the other files in /scripts do.

+1. This script should not be named .sh if it isn't a shell script -- that's just wrong.

I see two reasonable ways to fix this:

1. Rename the file to run-tests.php
2. Do not rename, but add "#!/usr/bin/env php" as the first line to match other scripts/ files

Both methods will break various automatic testing systems, but leaving this bug makes running tests unnecessarily confusing for developers. This is the ideal time to get this into 8.x, since we're early in the development process.

I think we should get this done in 8.x, and soon.

We don't have that for a reason: we need to use the same version of PHP and under the same permissions as the web server. This is not trivial to do, so simply renaming to .php will not make running tests less confusing. Also, if we rename to .php we need to protect against running under the web server.

I think we have to do something about this. I found it incredibly misleading.

Suggestions in IRC:

* No suffix (confuses Mac and Windows but I still think it's good)
* .php with a .htaccess to protect it (IIS doesn't respect .htaccess)
* .php.txt (makes sense to me)
* .php_

I think I'd prefer the standard .php extension with this check to prevent web server execution:

if (php_sapi_name() !== 'cli') {
  exit();
}

Regarding the shebang, I think we can add it and that will help the majority of users who only have one version of php installed. More advanced users can still invoke a different interpreter directly, since in that case the shebang gets ignored as if it were a comment.

I like that.

reffered to #8
I don't know did I make it in proper way but I just try it

heh. Not surprisingly, the test bot's operation depends on this file's name.
Output: [Could not open input file: ./core/scripts/run-tests.sh].

You might want to try a symlink or something. It will be years to make the transition... Not sure if one can check a symlink into git.

Yes git can track symlinks easily. Although, given the handful of other ".sh" files in core/scripts, maybe the more consistent change would be to add the shebang. Of the 8 scripts shipped with core, run-tests.sh is the only one without #!/bin/sh or #!/usr/bin/env php.

It's just so *wrong* for it to have .sh though. SO WRONG!!!!

#15 rfay: yeah I agree it's looks really wrong to have .sh for php script .. I'll try with simlink..
And one question, why did you say: "It will be years to make the transition..." why it should take years to do it??
There must be a way to create patch with file renaming.

It had a shebang...but that was removed since it broke for windows folks...and now we all suffer together. We can add a file_exists() to check in both places rather the muddy up repos with symbolic link.

@sobi3ch, I guess I was saying that we still have 6.x and 7.x in testing. This would have to be added to the testbot as an item to be found. But I think a symlink is the key here.

#1760928: Allow for #655178 to rename run-tests.sh to run-tests.php

I can go ahead, commit and deploy this Wednesday with other changes as long as there is a consensus on the filename.

boombatower: Thanks, do you have any more information on the Windows problem with shebangs? I found one old bug in a 5.3 alpha but it was fixed a long time ago: https://bugs.php.net/bug.php?id=47089

So here's one way:

Relevant issues
#474270: Rename run-tests.sh to run-tests.php
#331580: Make run-tests.sh find PHP on Windows & Linux

Thanks for the links, if I'm reading right at least one problem with the shebang on this script (not necessarily windows-related) is that some users want to autodetect the PHP binary path with getenv('_'), and that only works if it's run like /usr/bin/php run-tests....

There's a PHP_BINARY constant but it's not available until PHP 5.4.

Well, related to that which is where the issue on windows came up I believe is that we had /usr/bin/php in shebang which didn't work for everyone. Obviously, there is env php and what not in shebang, but you can see the context.

so maybe there should be seperate files for win and *nix like:
run-teset.bat <-- fire .php_ file (only windows)
run-tests.sh <-- fire .php_ file (only *unix)
run-test.php_

Especially if you guys check first lines in scirpts/ directory in D6 and D7:

scripts/ in D6:
============
$ find . -type f | xargs head -1
==> ./cron-lynx.sh <==
#!/bin/sh
==> ./code-clean.sh <==
#!/bin/sh
==> ./drupal.sh <==
#!/usr/bin/php
==> ./cron-curl.sh <==
#!/bin/sh
==> ./code-style.pl <==
#!/usr/bin/perl -w

scripts/ in D7:
============
$ find . -type f | xargs head -1
==> ./cron-lynx.sh <==
#!/bin/sh
==> ./code-clean.sh <==
#!/bin/sh
==> ./drupal.sh <==
#!/usr/bin/env php
==> ./password-hash.sh <==
#!/usr/bin/php
==> ./dump-database-d7.sh <==
#!/usr/bin/env php
==> ./generate-d6-content.sh <==
#!/usr/bin/env php
==> ./test.script <==
This file is for testing purposes only.
==> ./run-tests.sh <==
<?php
==> ./cron-curl.sh <==
#!/bin/sh
==> ./dump-database-d6.sh <==
#!/usr/bin/env php
==> ./generate-d7-content.sh <==
#!/usr/bin/env php

.. so it seems ALL scripts use shebang with env OR direct path to command. Maybe I'am lost but I don't see the reason why only just run-tests script need to be all cross platform compatible??? Can you explain me this please.

I would agree with you that it doesn't make sense to special case run-tests.sh. The reason it was is because people _actually use_ run-tests.sh and no one uses the others, people use drush or not those scripts at all. So people ran into the issue and then it got hot fixed (I was never a fan).

Making these scripts consistent would seem to make a lot of sense. I also got Dries (a while back, script I was working on never panned out) to agree to allow drush scripts in core so perhaps that would be useful here. Seems like a fairly reasonable thing to do and solves the comparability issues.

Correct me if I'm wrong. You suggest (and Dries was agree with you) that we should convert all .sh to .drush script, and make drush command line mandatory? For me sounds cool, but is it not to much (or make sense at all) for simple script to bootstrap all drupal stack before it's run? so many questions..

Dries simply agreed that it made sense (after a discussion with several key core folks) to allow drush scripts in core. That doesn't mean everything has/will be, but that we can use drush scripts in core. There is a patch in the queue to add core to the path that drush searches for for scripts (we will need to update for d8 /core if we want to do this).

Forgot it was actually committed, #1317562: Include core modules in command search path.

Anyway we will probably need to update that.

OK I see, so how should this look like on the end, in working enviroment?
$ drush scr run-tests.drush
if so there is an bootstrap issue. We often want to run script in DRUSH_BOOTSTRAP_DRUSH (simplest; Only bootstrap Drush, without any Drupal specific code) mode without bootstrapping unnecessary stuff. I think there should be some configuration (or flag in shebang) to run in appropriate bootstrap mode. Somthing like e.g.:

<?php
  define('DRUSH_BOOTSTRAP', DRUSH_BOOTSTRAP_DRUSH);

Then you could run it with:
$ drush scr script.drush
and be sure it's in proper bootstrap mode. More about boostraping: http://drush.ws/docs/bootstrap.html . I'm not very good in drush boostraping so maybe I'm doing something stupid.

I realize there is shebang handle by drush as well so you can start your file like this: #!/usr/bin/drush but again we run to windows issue, so probably above example with define() is better.

What's your thoughts?

We need to alter drush to look for scripts in the core/scripts dir if it does not already. drush uses a hook to define command and you can include the bootstrap level. http://drupalcode.org/project/drush.git/blob/refs/heads/7.x-5.x:/docs/dr...

Drush also already has a run-tests command which perhaps we want to copy into core and tweak. http://drupalcode.org/project/drush.git/blob/refs/heads/7.x-5.x:/command...

I haven't looked at it so not sure if it would even remotely work (I just know it exists). Having this in core though would allow us to replace current script and keep in sync so testbot can use it.

Wow! seems I need to learn more about drush before I continue this discussion. I need to go sleep..

@boombatower: hey can you provide link to this patch? I would like to look at it.. cheers

Link to what patch? Not sure if you were referring to the "script" I mentioned, but that was referring to run-tests.sh being replace by drush command that testbot can use.

Theoretically, the bot could use drush right now...best with stable 6.x, 7.x, but not 8.x since everytime the testing system is changed drush needs to be updated in the same patch. So moving the drush command in core would allow just that and replace the run-test.sh code. Most likely would require some sort of feature merging since I doubt both work the same.

So moving the drush command in to the core means installing drush in /core/drush/ directory or something else? I just want to help but I don't know from which point to start..

Drush will follow its standard installation instructions. The only file in core will be the drush command definition itself.

The title and summary don't match.

Could we please do this? PhpStorm almost freeze every time open this file.

+1

Isn't this a BC break?

Isn't this a BC break?

run-tests.sh is still in place but marked as deprecated. So this should not break anything.

Or was it specially made?

@vaplas, no I didn't mean to change it. Thanks for the review and the patch!

it is not executable anymore so I think mod change is fine.

Why not use PHP shebang in this file?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled against 10.1.x

I think that run-tests.sh is the old way of working because we were not using phpunit? Are we keeping this supported,
renaming this at this point in time doesn't feel needed/useful.

Moving to the testing component.

Re: #76

We use run-tests.sh in the CI right now, and it would be great if it were more maintainable.

IIRC the reason it's got the .sh extension is that PHP won't discover the file, and so that htaccess type configuration can exclude it more easily. So it's a security through obscurity type scenario.

Changing the file name also means changing the CI system, and while it doesn't seem like we'd need to alter too much, we'd end up with different file names for different versions of core. A CI system maintainer could speak to whether that effort would be worth it, and what the future plans are for switching to a different test runner potentially.

In my ideal world this script would have been refactored into a symphony console based app, and thus much more maintainable and less ambiguous when it comes to file names. #2624926: Refactor run-tests.sh for Console component. But I fear that ship has sailed as well, because we have always been 'just about to' switch over to something else.