With drupal_detect_baseurl(), D8: now unused - remove it, D7: make use of request_uri() and test

Why call request_uri() instead of $_SERVER['REQUEST_URI'] ?

I think the documentation for request_uri says it all;

"Since $_SERVER['REQUEST_URI'] is only available on Apache, we generate an equivalent using other environment variables."

Re-roll patch from #1.

Looks good to me. Does this function have unit tests? We could maybe test a few patterns with a dummy $_SERVER, I guess.

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

Here is the rerolled patch.

Here is the test for this issue.

Fixed test $_SERVER variables in setUp().

Awesome, thanks for the tests! There's a couple minor things with the code style that distracted me a bit:

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1827,6 +1827,57 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+ * Test for drupal_detect_baseurl().
...
+   * Test valid base urls.

Should begin "Tests." (Yeah, that's different from the rest of the file, but we're in the process of cleaning it up as a part of #1310084: [meta] API documentation cleanup sprint). ;)

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1827,6 +1827,57 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+ * @see http://drupal.org/node/226873
+ * "Since $_SERVER['REQUEST_URI'] is only available on Apache, we generate an
+ * equivalent using other environment variables."

I think we could probably leave off the @see here, since it can be gotten via git blame. Instead, let's remove the quotes and say:
Since $_SERVER['REQUEST_URI'] is only available on Apache, we use drupal_detect_baseurl() uses request_uri() to generate an equivalent using other environment variables.

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1827,6 +1827,57 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+      'name' => 'Valid Base Url',
+      'description' => "Performs tests on Drupal's valid base url function.",

Very minor point, but URL should be all in caps.

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1827,6 +1827,57 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+    // Save $_SERVER variable.

I'd clarify this a little: "Save original $_SERVER variable." Then, above the next section, add a comment that says something like: "Create a dummy $_SERVER variable for testing."

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1827,6 +1827,57 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+  ¶

Teensy bit of trailing whitespace here.

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1827,6 +1827,57 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+    $this->assertEqual($url, $request_uri, t('The base URL detecting is valid.'));

Maybe reword a little to "The detected base URL is valid." Also, we can probably leave off the t(). See #500866: [META] remove t() from assert message.

I'd aslo suggest an inline comment or two in testValidBaseUrl() to explain what we're doing.

If you roll a new patch, could you also re-upload the (new) test-only version just to make it clear that it fails for the "right" reason this time?

Great job on this. Thanks for picking up the issue so quickly. :)

Hmm, looks like there's still something else going on. I'll try it locally.

Yeah, locally the test ran succesfully. After the first failed server test I tried to set up a test environment similar to real one.

Hate to say this given the work that's already gone into this issue, but for Drupal 8, at least, couldn't we just delete the drupal_detect_baseurl() function entirely? It doesn't seem to be used anywhere in core, and it's hard to see why it would be useful elsewhere since we have $GLOBALS['base_url'] instead...

For Drupal 7 backport I guess we have to keep it, but it might not be worth too much effort getting the tests to work if they're troublesome (if the function is on its way out anyway).

That said, I'm guessing the test fails due to this:

+    $_SERVER['SCRIPT_NAME'] = 'index.php';

That won't work if Drupal is being run in a subdirectory (such as http://localhost/drupal), and it failed for me when I tried it locally because of that... Maybe the testbot runs in a subdirectory also?

Meanwhile the "Undefined index: HTTPS" PHP notice in the tests seems to be a straight-up bug in drupal_detect_baseurl() itself. I'm not sure about the other PHP notices; perhaps they are also.

I agree with you, I have just found one calling the drupal_detect_baseurl() and seem replaceable with $GLOBALS['base_url'].

.

If there's no use for the function, this patch removes drupal_detect_baseurl().

#18: request_uri-226873-18.patch queued for re-testing.

For the D7 backport, the patch in #9 needs work but it's a good place to start. #18 seems sane for D8. This counts as an API change in D8, so once this is committed we'll need a change notice as well. Docs on change records.

Committed/pushed to 8.x, thanks!

Sorry, typed too fast, no longer applies.

Rerolled #18.

Thanks for the quick re-roll. Actually committed/pushed this time. Back to 7.x for backport.

I made a port of the patch from #9 for 7.x including the suggested minor changes in #10, I'm assuming this will still fail, but maybe get this moving again.

Looked at this a bit more this morning, it looks like this function just doesn't do what is intended. Also, it is never called anywhere that I can find in D7 either.

I could have sworn this test was passing for me yesterday, but looking today it is not & it's due to $_SERVER['SERVER_NAME'] not being set. I looked into how $base_url is created in drupal_settings_initialize(), and it is using $_SERVER['HTTP_HOST']; perhaps drupal_detect_baseurl has just been broken/unused for a long time & not noticed?

All sorts of issues with this function, apparently.