password-hash.sh is broken

This works

It should be possible to write a phpunit test for this, right?

I think it's even more broken now - the include file is gone/moved.

@klausi - do we want a unit test that shells out? I agree we need a test, but not sure how.

This should be a simple fix, but we shouldn't ship with this being totally broken.

this basically fixes the script (is it the best fix?)

This uses a unit test to exec the shell scripts - let's see what testbot thinks.

Chatted with Crell and others in IRC, and no one had a better idea about how to write a test to make sure we don't break a script.

As exec() is disabled on some environments I think it would be cool to conditionally skip those tests in that case. Something like this (untested):

// In setUp()
// Skip this test if exec() is disabled.
$disabled_functions = explode(',', ini_get('disable_functions'));
if (in_array('exec', $disabled_functions)) {
  $this->markTestSkipped('exec() is not available');
}

Edit: Forgot explode()

Agreed with #8. Otherwise this looks fine.

+++ b/core/tests/Drupal/Tests/Core/Utility/ScriptTest.php
@@ -0,0 +1,56 @@
+  }
...
+
+  /**
+   * Tests password-hash.sh.
+   */
+  public function testPasswordHashSh() {
+    $cmd = 'core/scripts/password-hash.sh xyz';
+    exec($cmd, $output, $exit_code);
+    $this->assertEquals(0, $exit_code, 'Exit code');
+    $this->assertContains('hash: $S$', implode(' ', $output));

Note that this means that we're running the script against the actual installation, which means that it would fail if there's no installation, we can't verify that the correct hash is created and while not this, other scripts could possibly result in changes on the live site. So we should probably at least clearly document it on the class/method, in case we add more tests later on.

@berdir has a good point, this could make bad things happen in the future. As this requires a Drupal installation, I think this should NOT be a PHPUnit test.

This is completely self-contained - doesn't cause problems anywhere with the system and you can't get to the fatal unless you already know what you're doing. Downgrading to major and removing beta-blocker tag.

@damiankloip - I agree it's not a unit test per-se, but what other framework do we have to run this?

@Berdir - we are only bootstrapping code, so I don't think it requires a full install. We have to do that, I think, to use the \Drupal class.

If we want to skip using the service container and directly use the default class, we could make this script run with no bootstrap at all.

I'm not saying there's anything wrong with the script, I was talking about the test.

All other tests, *especially* unit tests, run in a completely isolated environment. This test is nothing more than a wrapper that calls out to a script, that script will then (attempt to) connect to your actual installation. And all I'm saying is that's a very uncommon behavior that you might not be aware of if you want to run or extend that test.

Damian's suggestion was to make it a simpletest test, as that's a bit closer to what this test is doing. Or we could simply document that in a very obvious way inside that test, as suggested by me.

Yes, as mentioned in IRC yesterday (berdir has basically summed this up in #15). Both of those scripts have a dependency on Drupal being installed.

So as outlined yesterday, you can run PHPUnit tests just with the code base, this would break that. So as I said, at least just make this DUTB implementation instead. This is why I don't think we should rely on some code comments. It would be a shame to not be able to run the unit tests in isolation because of this one test.

I don't see what the issue is with that? or the confusion for that matter?

Sure, in part my question was whether DUTB was deprecated (was answered in IRC as not deprecated). And in fact we can just use UnitTestBase it seems.

I would say that is more likely to get deprecated before DUTB, but OK :)

@damiankloip - well, it seems DUTB extends UTB, and the doc on it indicates that UTB is for tests that never need to access the database, etc. Anyhow, it seems like both with persist in Drupal 8 at this point.

Fair enough. Anything left in UTB could easily just move into DUTB but whatever. I think this is fine!

Yay, tests. :D

Committed and pushed to 8.x. Thanks!

The shebang line change should have been backported in this issue. Here is the issue to fix it in D7: #2559335: password-hash.sh is hardcoded to use "/usr/bin/php"