Tried running tests on my shared hosting account (the server is a tad more powerful than my laptop). The server has an open_basedir restriction that interferes with the following 2 curl options:
CURLOPT_COOKIEJAR => NULL
CURLOPT_FOLLOWLOCATION => TRUE
I got round the first by setting the cookie jar to a random filename e.g.
protected $cookie_file = './cook';
The file didn't seem to get created, no errors were reported ...
However the FOLLOWLOCATION setting is more of a problem since PHP reports thatthis is incompatible with open_basedir. Running the syslog tests with this option set to FALSE I get a test failure after each POST (2 in my case), which makes sense. I guess simpletest would need to do handle the redirections itself?
Comments
Comment #1
Dave ReidMarked #360417: Bad curl_setopt() when open_basedir restriction is in effect. as a duplicate of this issue.
Comment #2
gpk CreditAttribution: gpk commentedUpdate:
#554106: Module enable/disable tests failing on testing servers temporarily set CURLOPT_FOLLOWLOCATION => FALSE, but it was reverted.
The other issue has a bit of useful info on this at http://drupal.org/node/360417#comment-1207025 - specifically see http://www.php.net/manual/en/function.curl-setopt.php#71313 (the other comment referenced on the php.net page seems to have gone..!)
Also (for my info) the cookie file is defined here:
http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/drupal_...
and then the jar is set here: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/drupal_...
Comment #3
sunHOLY COW.
I spent like the entire day to figure out what's wrong with my server, checked cURL installation/configuration, PHP configuration, extensions, DNS resolver, hacked drupal_web_test_case.php inserting
CURLOPT_RETURNTRANSFER => TRUE
in various places (which at least made GET requests return more than 0-1 bytes) and whatnot...So, yeah, hours later I find this issue, altered my httpd.conf:
and suddenly it works. :-/
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedCreated a patch for simpletest.install adding a requirement saying that open_basedir has to be set to none!
Comment #5
sunGood idea.
Trailing white-space here.
Wrong indentation.
Missing closing brace for if statement.
This review is powered by Dreditor.
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedFixed the issues, attaching updated patch file.
Comment #7
sunStill trailing white-space here.
Proper capitalization? Perhaps also prefix with "PHP", so people know what this belongs to?
SimpleTest was partially renamed to "Testing", i.e. as in "testing framework".
A further rename of the internal module name is likely to happen.
In any way, this user-facing message should not use "SimpleTest".
This review is powered by Dreditor.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedFixed thoose issues, attaching updated patch.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedFound an indentation error, corrected in attached patch.
Comment #10
sunLooking at the other keys, let's rename this to "php_open_basedir".
Let's rename this to "PHP open_basedir value".
This logic looks wrong/reversed...?
Missing trailing period/full-stop.
Let's also re-phrase that into:
The testing framework requires PHP's open_basedir to be set to 'none'. Please check your webserver configuration.
I'm on crack. Are you, too?
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedAnother update! :)
Comment #14
sunYou need to use double-quotes here.
I'm on crack. Are you, too?
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedHad used wrong CVS release for previous patch. Attaching proper one.
Comment #16
sunwell done.
Welcome to the core development team! :)
Comment #17
chx CreditAttribution: chx commentedWelcome indeed -- I gave nenne this issue on IRC and some instructions -- thanks sun a lot for guiding him towards this nice solution!
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you both for the help! Learned alot from this!
Comment #20
sunComment #21
webchickGreat work, nenne! :) Some comments...
Would it be possible to expand this very slightly to include why? Or at least what happens when this is not set properly? I know this creates inconsistency with the other ones I can guess at (cURL is needed for the browser, DOM for HTML/XML parsing...) I have no idea why open_basedir would be screwing things up. I've never in my life needed to look at that setting before, in all the PHP development I've done.
Alternately, should we be investigating why open_basedir is screwing things up? Are we simply identifying a symptom rather than treating the cause?
I'm on crack. Are you, too?
Comment #22
gpk CreditAttribution: gpk commented>I have no idea why open_basedir would be screwing things up.
I think the answer might be in the original post?
>Are we simply identifying a symptom rather than treating the cause?
Yes; I was planning to open a separate issue for adding open_basedir support once/if this workaround gets committed. Since open_basedir support might take a while to arrange (because it's something few core devs need ... they don't use shared hosting) I think it would be helpful to commit this patch, perhaps with an additional comment explaining why open_basedir is not currently supported) to make sure that others avoid the pickle that sun got into :P, until such time as the symptom is properly treated.
I guess it all depends a bit how important open_basedir support is deemed to be. As things stand at the moment, a large proportion of Drupal 7.x users will be unable to run tests without either changing web host or setting up a local dev environment. Hardly the best way of encouraging people to get involved in reviewing patches/testing/best practices etc.!
Comment #23
gpk CreditAttribution: gpk commentedSlight update including:
- comment added in response to #21, which also references new issue #674304: Make testing compatible with open_basedir restriction
- 'none' is how you turn off open_basedir in httpd.conf, but is not a valid value
Comment #24
sunThanks! It would really be nice to get this one in to save other developers from the nightmares I had to go through. Even if it's only temporary until the real cause can be fixed (if it can be fixed at all).
s/not to be set./to be disabled./
9 days to code freeze. Better review yourself.
Comment #25
gpk CreditAttribution: gpk commented> s/not to be set./to be disabled./
Yes I think I was trying to reflect the fact that open_basedir is usually disabled by default ... your php.ini must have turned it on or maybe another directive somewhere in the Apache config. However since the message is only displayed if there is a problem your suggestion makes sense.
>if it can be fixed at all
I don't see why not.. requires 2 curl options to be changed ... NULL for cookiejar (which looks like a bit of a "trick" anyway), and also redirects would need to be processed explicitly instead of automatically.
Comment #26
suns/See also/@see/
Following the new wording, we should use "PHP open_basedir restriction" along with "Enabled" and "Disabled" here as well.
I think that we can remove the url() here...?
Afterwards, RTBC.
9 days to code freeze. Better review yourself.
Comment #27
sunIn addition, "SimpleTest" should not appear in user-facing text. We replaced it with "The testing framework" earlier.
9 days to code freeze. Better review yourself.
Comment #28
gpk CreditAttribution: gpk commentedLargely fair enough ;)
re. url() and SimpleTest, I was using those formats for consistency with the rest of http://api.drupal.org/api/function/simpletest_requirements/7. So what to do?
Comment #29
sunThe other external URLs are not using url(). The other strings should be updated to not use "SimpleTest", but that rather belongs into a separate issue.
Comment #30
gpk CreditAttribution: gpk commentedDoh! I was using the link to the PHP info page as an example!
Comment #31
sunAwesome!
Comment #32
webchickThanks. I ran this past Jimmy last night and he was cool with it.
Committed to HEAD.
Comment #33
gpk CreditAttribution: gpk commentedThanks webchick!
@sun: is it actually too late to change, for 7.x, the other mentions of "Simpletest" to "The testing framework" in simpletest_requirements()..?
[update] They were fixed here #678050: Strings tidy-up for simpletest_requirements
Comment #35
leonardjo CreditAttribution: leonardjo commentedThe current wording of the error in the status page gives the impression that open_basedir restrictions are a bad thing. This sends entirely the wrong signal to the user.
The error message should be reworded to indicate removing open_basedir restrictions actually decreases system security and that open_basedir restrictions should be reinstated when no testing is being done.
Comment #36
nedjoRelated issue: #357469: file_save_upload() fails to handle open_basedir restriction.
Comment #37
salvis@leonardjo: I couldn't agree more!
Asking the admin to turn off the open_basedir restriction is a work-around (better than leaving him in the dark about why Testing is not working), but it's not a solution.
open_basedir is a useful security feature.
Comment #38
Damien Tournoud CreditAttribution: Damien Tournoud commentedopen_basedir is part of the deprecated (and removed from trunk) security mascarade that is the Safe Mode feature. The current wording is fine. No one in his right mind should need to run with open_basedir.
Comment #39
salvis@Damien Tournoud: Please provide pointers to some factual evidence that supports your strong words.
Comment #40
Dave ReidAll I've tried searching for just now shows that open_basedir is a different item from Safe mode, which the latter is indeed deprecated, but not the former. Other things like disable_functions and disable_classes show nothing about being a part of safe mode or being deprecated.
Comment #41
catchComment #42
sunI was equally confused/surprised by @Damien Tournoud's follow-up, and am pretty sure that
safe_mode
was deprecated in favor ofopen_basedir
.Docs on safe_mode contain a bold warning about being deprecated, also referring to open_basedir as replacement.
Comment #43
sunSorry, x-post. Didn't someone post a workaround for underlying cURL problem that needs open_basedir to be unset?
Comment #44
maths222 CreditAttribution: maths222 commented#30: simpletest_openbasedir.patch queued for re-testing.
Comment #46
sunComment #47
regilero CreditAttribution: regilero commentedopen_basedir is certainly not a "security mascarade", it the first step of a good installation. Am I wrong or none ever tried to fix the problem?
We can find on the net some workaround where the FOLLOWONLOCATION is handled by some curl php code in case of open_basedir restrictions http://stackoverflow.com/questions/2511410/curl-follow-location-error
This seems a lot more serious than simply asking for any PHP security settings removal before running tests (how do you test the security restrictions then?), it's like a big "please chmod 777" :-)
Comment #48
fantom84 CreditAttribution: fantom84 commentedsubscribe
removing open_basedir is a bad idea
Comment #49
Rabikumar CreditAttribution: Rabikumar commented#6: updated-323110-simpletest-openbasedir-issue-D7.patch queued for re-testing.
Comment #50
rehanmerchant CreditAttribution: rehanmerchant commented#4: 323110-simpletest-openbasedir-issue-D7.patch queued for re-testing.
Comment #51
chx CreditAttribution: chx commentedThis issue is a mess lately. It's not clear what it is about and noone is helping by filing a clear report on what's desired instead just clicks around destroying useful past testing results.
As far as I can see, both conflicting settings has been resolved since and so we can remove this check, effectively reverting. This requires someone to do more work than write a nonsense comment: one needs to remove the requirements check, set open_basedir and run tests. I think they would pass now.
Comment #52
chx CreditAttribution: chx commentedMaybe not novice because of the php.ini edit requirement.
Comment #53
MrHaroldA CreditAttribution: MrHaroldA commentedI ran into the open_basedir requirement too, and found this in
simpletest.install
:#674304: Make testing compatible with open_basedir restriction and this issue both mention that these cURL options conflict with open_basedir:
When searching Drupal's codebase for these options, I only found these in drupal_web_test_case.php
... so I guess the whole requirement can be omitted from
simpletest_requirements()
? I've attached a patch that removes it.Comment #54
MrHaroldA CreditAttribution: MrHaroldA commentedComment #55
BarisW CreditAttribution: BarisW at LimoenGroen commentedHere is a re-roll against current HEAD.
Comment #57
BarisW CreditAttribution: BarisW at LimoenGroen commentedDuh
Comment #58
BarisW CreditAttribution: BarisW at LimoenGroen commentedComment #59
BarisW CreditAttribution: BarisW at LimoenGroen commentedSorry for the noise to re-activate the testbot.
Comment #60
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #62
BarisW CreditAttribution: BarisW at LimoenGroen commentedThe previous patch was made against 8.0.x-dev, this one is against 8.1-dev
Comment #71
joseph.olstadD8 fixed.
Backport to D7 is ready, see backport issue:
#2926150: D7 Remove the open_basedir requirement check
Comment #72
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis is not fixed - the code which #62 removes is still in Drupal 8.
Comment #73
joseph.olstadThanks David for verifying.
#47 says it all, this open_dir check needs to be removed. It was put in as a hack for something that was fixed long, long ago.
Comment #76
joseph.olstadD8 patch above #62 should go in
#323110-62: Remove the open_basedir requirement check
D7 here: #2926150-9: D7 Remove the open_basedir requirement check
Comment #77
PanchoRerolled #62 against 8.7.x-dev.
Don't know if simpletest can properly test itself, so I'm also going to test it manually on shared hosting.
Comment #78
PanchoPretty slow on shared hosting but it works flawlessly despite of
open_basedir
restrictions being active. :)Let's some more people test it on their systems, and then we should finally go forward.
Comment #80
Mirroar CreditAttribution: Mirroar at werk21 commentedI can confirm that the patch from #78 works and looks good to me. I was able to enable the testing module and run tests successfully. Carefully setting this to RTBC.
Comment #82
catchGlad we do separate 7.x and 8.x issues these days.
Committed f9f013c and pushed to 8.8.x. Thanks!
Comment #83
kim.pepperCan we update the IS as to why we can safely remove this?