Add ltrim($path, '/') in drupalGet method

Patch uploaded contains ltrim in drupalGet() and tested. Please review and post feedback.

Thanks

I like the general idea, one less thing to worry about.

Note: the failures are caused by you calling ltrim to $url if url is an object, see the error message:

ltrim() expects parameter 1 to be string, object given

Patch from #10 would break Url::isExternal in case a string path starting with // is given. Moving this part into buildUrl() method.

@mxh
Please try to avoid taking over someone's issue when they assigned themselves a short time ago. We all want to avoid redundant work, don't we?

Sorry, I have overlooked the fixed assignment.

No worries :)

Patch from #10 would break Url::isExternal in case a string path starting with // is given. Moving this part into buildUrl() method.

Nice catch!

Do you think we can add some little test which calls explicitely with a starting slash?

This one might fail as I'm currently unable to have an environment which executes the test. Not sure whether this is sufficient / at the right place as well.

Argh, wrong assert.

That's some serious amount of test coverage. Thank you!

Great work on the tests!

I've uploaded a test-only patch, which should presumably fail on the leading slash case in HEAD? In general it's always a good idea to provide a test-only patch along with the fix.

Let's make it actually fail

The added test testBuildUrl() should also presumably fail in case add_ltrim_path-2581557-10.patch has been applied.

Ha, #fail indeed xjm. :P

So, it didn't fail.

Is this change still needed? The issue was filed as a followup before release.

I could easily imagine that this works on BrowserTestBase but maybe not on WebTestBase?

I don't know why the test isn't actually covering the change being made in add_ltrim_path-2581557-20.patch.

I'm wondering, whether this part

<?php
$uri = $path === '<front>' ? 'base:/' : 'base:/' . $path;
?>

shouldn't include the slash at the end, i.e. it should be instead

<?php
$uri = $path === '<front>' ? 'base:/' : 'base:' . ltrim($path, '/');
?>

or another possible alternative:

<?php
$uri = $path === '<front>' ? 'base:/' : 'base:' . $path;
?>

What do you think?

It's also possible that it's actually not needed anymore. It seems test implementations don't struggle with this since release, so we could close this issue and reopen it in case there is an explicit need for it? Could still add the tests though.

It's also possible that it's actually not needed anymore. It seems test implementations don't struggle with this since release, so we could close this issue and reopen it in case there is an explicit need for it? Could still add the tests though.

I'm inclined to agree with this and rescope this issue to just add the additional testcoverage.

@borisson_
Sure, let's do that.

Awesome, more tests! Great work @dawehner. Thanks so much for picking this up.

+++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -67,6 +67,18 @@ public function testGoTo() {
+  public function testDrupalGet() {
+    $this->drupalGet('test-page');
+    $this->assertSession()->statusCodeEquals(200);
+    $this->drupalGet('/test-page');
+    $this->assertSession()->statusCodeEquals(200);
+    $this->drupalGet('/test-page/');
+    $this->assertSession()->statusCodeEquals(200);
+  }

Is the URL going to be normalised? Should we add a $this->assertUrl() for each case too?

It doesn't do normalisation, which is reflected in the interdiff.

Thanks @dawehner.

Credited @JeroenT for creating the issue.

Committed and pushed 9e9ee75ba4 to 8.6.x and 83ab546116 to 8.5.x. Thanks!