Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some tests are not removing base_path from paths before attempting to fetch them. #4656

Open
bradbulger opened this issue Sep 28, 2020 · 8 comments · May be fixed by backdrop/backdrop#3359
Open

Comments

@bradbulger
Copy link

For example, in testBlockBasics() in layout.test, it parses out a link from generated HTML results and calls backdropGet() on the parsed path of the original URL. When the test is running in a subdirectory, like https://my.server/testing/backdrop/, the link's path includes the base path of the site. The url() function prefixes the path with base_path() resulting in an invalid URL. The test should be removing the base path first.

You can recreate this by running LayoutBlockTest in any site running in a subdirectory.

@bradbulger
Copy link
Author

It's easy enough to add a line of code to each instance to remove base_path(), but this bundle of steps occurs multiple times in several tests, so I wonder if it'd be preferable to add a function to backdrop_web_test_case.php or some other base level?

@bradbulger
Copy link
Author

For example, there's this bit from the Dashboard test:
$disable_link = $this->xpath('//a[starts-with(@href, "/admin/structure/layouts/manage/dashboard/disable")]'); $disable_url_parts = backdrop_parse_url($disable_link[0]['href']); $this->backdropGet($disable_url_parts['path'], $disable_url_parts);
Both the construction of the parsed URL parts array and the path produce errors when this is run in a subdirectory. These steps - parse a URL, get the path from the parsed results - occur multiple times in several tests. So we could create some kind of href_to_parts() function to handle those. But it might be easier to locate problems if the errors are occurring directly in the test code. Like in this case, the xpath() call is failing so there's a bad array reference error as well as the failure on backdropGet().

@indigoxela
Copy link
Member

Maybe one of the first questions might be: should Simpletests consider being run in subdirectories? I can't answer that.

@bradbulger
Copy link
Author

I can't see how not. If anything, the similar problem on #2397 makes me realize I need to check these with clean URLs disabled as well.

@bradbulger
Copy link
Author

I was looking for other cases where a function to remove the base path might be useful and noticed that in views_plugin_style_jump_menu.inc it does this using backdrop_substr() and backdrop_strlen(). Are paths potentially multibyte strings? I don't know much about the subject.

@indigoxela
Copy link
Member

Are paths potentially multibyte strings?

Path aliases can be, if transliteration is off.

@bradbulger bradbulger linked a pull request Oct 6, 2020 that will close this issue
@bradbulger
Copy link
Author

I created a kind of maximal PR to fix this. I see other places where people are solving this problem, so I created the code to find and remove the base path as a common function, internal_path(). In some of those other places, i saw that it was using the Backdrop PHP wrapper functions to handle multibyte strings, and that seemed to make sense. So I added a backdrop_strpos() to go along with strlen() and substr().

The other approach would be to just make this a simpletest fix and add a simpler method to BackdropWebTestCase. See what you all think.

@bradbulger
Copy link
Author

A third alternative would be to just add a line that does path = substr(path, strlen(base_path())); in all of the places where this is an issue, skipping the test that base_path() is there because we "know" it is. I expect that would work, and would be the minimal change to fix the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants