Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
besides that it tries to include the dead password.inc, it blows up
PHP Fatal error: Class 'Drupal' not found in /var/www/d8/core/includes/bootstrap.inc on line 2118
Comment | File | Size | Author |
---|---|---|---|
#17 | 2108573-17.patch | 2.32 KB | pwolanin |
#17 | increment.txt | 2.42 KB | pwolanin |
#7 | 2108573-7.patch | 2.21 KB | pwolanin |
Comments
Comment #1
swentel CreditAttribution: swentel commentedThis works
Comment #2
klausiIt should be possible to write a phpunit test for this, right?
Comment #3
pwolanin CreditAttribution: pwolanin commentedI 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.
Comment #4
pwolanin CreditAttribution: pwolanin commentedThis should be a simple fix, but we shouldn't ship with this being totally broken.
Comment #5
pwolanin CreditAttribution: pwolanin commentedthis basically fixes the script (is it the best fix?)
Comment #6
pwolanin CreditAttribution: pwolanin commentedComment #7
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #8
tstoecklerAs exec() is disabled on some environments I think it would be cool to conditionally skip those tests in that case. Something like this (untested):
Edit: Forgot explode()
Comment #9
Crell CreditAttribution: Crell commentedAgreed with #8. Otherwise this looks fine.
Comment #10
BerdirNote 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.
Comment #11
damiankloip CreditAttribution: damiankloip commented@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.
Comment #12
catchThis 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.
Comment #13
pwolanin CreditAttribution: pwolanin commented@damiankloip - I agree it's not a unit test per-se, but what other framework do we have to run this?
Comment #14
pwolanin CreditAttribution: pwolanin commented@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.
Comment #15
BerdirI'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.
Comment #16
damiankloip CreditAttribution: damiankloip commentedYes, 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?
Comment #17
pwolanin CreditAttribution: pwolanin commentedSure, 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.
Comment #18
damiankloip CreditAttribution: damiankloip commentedI would say that is more likely to get deprecated before DUTB, but OK :)
Comment #19
pwolanin CreditAttribution: pwolanin commented@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.
Comment #20
damiankloip CreditAttribution: damiankloip commentedFair enough. Anything left in UTB could easily just move into DUTB but whatever. I think this is fine!
Comment #21
webchickYay, tests. :D
Committed and pushed to 8.x. Thanks!
Comment #23
cilefen CreditAttribution: cilefen commentedThe 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"