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.
Problem/Motivation
Follow-up for #2575869-70: Remove all remaining usage of deprecated UrlGeneratorInterface::generateFromPath() and delete it.
+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -412,7 +412,7 @@ protected function drupalGet($path, array $options = array()) {
+ $url = Url::fromUri('base:/' . $path, $options)->toString();
+++ b/core/modules/simpletest/tests/src/Functional/BrowserTestBaseTest.php
@@ -34,7 +34,7 @@ public function testGoTo() {
- $this->drupalGet('/test-page');
@@ -46,7 +46,7 @@ public function testGoTo() {
- $this->drupalGet('/form-test/object-builder');
@larowlan:
Instead of changing the ->drupalGet calls, could we just use ltrim($path, '/') in drupalGet method
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff-2581557.txt | 835 bytes | dawehner |
#35 | 2581557-35.patch | 983 bytes | dawehner |
#32 | 2581557-32.patch | 810 bytes | dawehner |
#23 | add_ltrim_path-2581557-20-FAIL-for-real.patch | 1.79 KB | dawehner |
#22 | add_ltrim_path-2581557-20.patch | 2.81 KB | xjm |
Comments
Comment #2
JeroenTComment #3
JeroenTComment #4
JeroenTComment #9
sorabh.v6Comment #10
sorabh.v6Patch uploaded contains ltrim in drupalGet() and tested. Please review and post feedback.
Thanks
Comment #12
dawehnerI 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:
Comment #13
sorabh.v6Comment #14
mxhPatch from #10 would break
Url::isExternal
in case a string path starting with//
is given. Moving this part intobuildUrl()
method.Comment #15
dawehner@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?
Comment #16
sorabh.v6Comment #17
mxhSorry, I have overlooked the fixed assignment.
Comment #18
dawehnerNo worries :)
Nice catch!
Do you think we can add some little test which calls explicitely with a starting slash?
Comment #19
mxhThis 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.
Comment #20
mxhArgh, wrong assert.
Comment #21
dawehnerThat's some serious amount of test coverage. Thank you!
Comment #22
xjmGreat 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.
Comment #23
dawehnerLet's make it actually fail
Comment #24
mxhThe added testtestBuildUrl()
should also presumably fail in caseadd_ltrim_path-2581557-10.patch
has been applied.Comment #25
xjmHa, #fail indeed xjm. :P
Comment #26
xjmSo, it didn't fail.
Is this change still needed? The issue was filed as a followup before release.
Comment #27
dawehnerI could easily imagine that this works on BrowserTestBase but maybe not on WebTestBase?
Comment #28
mxhI don't know why the test isn't actually covering the change being made in
add_ltrim_path-2581557-20.patch
.Comment #29
mxhI'm wondering, whether this part
shouldn't include the slash at the end, i.e. it should be instead
or another possible alternative:
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.
Comment #31
borisson_I'm inclined to agree with this and rescope this issue to just add the additional testcoverage.
Comment #32
dawehner@borisson_
Sure, let's do that.
Comment #33
borisson_Awesome, more tests! Great work @dawehner. Thanks so much for picking this up.
Comment #34
alexpottIs the URL going to be normalised? Should we add a
$this->assertUrl()
for each case too?Comment #35
dawehnerIt doesn't do normalisation, which is reflected in the interdiff.
Comment #36
alexpottThanks @dawehner.
Comment #37
alexpottCredited @JeroenT for creating the issue.
Committed and pushed 9e9ee75ba4 to 8.6.x and 83ab546116 to 8.5.x. Thanks!