Regression: Test failures with clean URLs
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
The fix for #8: Let users cancel their accounts revealed a bug in Simpletest and accidentially broke some tests, e.g. Aggregator > Add feed functionality and Book > Book functionality. The bug only occurs when using clean URLs. I don't know why this bug wasn't detected by the test bot (assume it uses clean URLs?).
The problem is this: DrupalWebTestCase::setUp() calls checkPermissions() prior to setting clean_url to $clean_url_original. checkPermissions() invokes user_perm(). With the fix #8: Let users cancel their accounts user_perm() now calls url(). At this point, clean_url is not yet set, so url() generates an "un-clean" URL. url() caches the value of clean_url in a static variable, so in later calls it still generates un-clean URLs even though clean URLs have later been enabled. This breaks some tests.
This patch is a subset of the patch for #238299: file_create_url should return valid URLs. As mentioned in that issue, I don't think this change has any noticable performance impact.
| Attachment | Size |
|---|---|
| clean-url-1.patch | 2.08 KB |
| Testbed results | ||
|---|---|---|
| clean-url-1.patch | passed | Passed: 8504 passes, 0 fails, 0 exceptions a href=http://testing.drupal.org/pifr/file/1/clean-url-1.patchDetailed results/a |
#1
I saw that too, but finally convinced myself that my dev environment was not working properly.
I consider that this is an issue in DrupalWebTestCase. Can't we fix it there?
#2
I think the patch looks reasonable, and would avoid confusing behavior. However, given that url() is called hundreds of times per page request, I wonder if there is a measurable performance impact here. Might be worthy some quick tests.
#3
I guess it would be possible to fix this specific case by changing DrupalWebTestCase::setUp(). However, it doesn't solve the general problem that you cannot change
clean_urlduring tests. I think this is generally useful, e.g. in tests like those suggested in #238299: file_create_url should return valid URLs. I don't think you can solve the general problem without changing url() itself. Or which approach would you suggest?WRT performance, I did some time measurements for 100,000 calls to url(). Without the patch, url() runs in 0.331 ms on my box, and with the patch it takes 0.344 ms, i.e. a small performance hit. With 100 url() calls on a page, the total running time would be increased by 1.3 ms.
An alternative approach would be to add the possibility to explicitly reset the static variable, e.g. using
url('', array('reset' => TRUE)). This reduces the running time to 0.335 ms.#4
Ok, given the benchmark in #3, let's go with the approach in the original patch. This unresetable static in url() has been bugging me for ages anyway.
#5
Committed to CVS HEAD. Thanks.
#6
Automatically closed -- issue fixed for two weeks with no activity.
#7
I'd like to open the discussion for back-porting this to 6.x. I'm running into problems trying to write a test for a 6.x contrib module:
<?php
function testNoCleanURLs() {
variable_set('clean_url', 0);
$this->testRedirects();
variable_set('clean_url', 1);
}
private function assertRedirect($request, $redirect) {
$this->drupalGet($request);
$this->assertEqual($this->getUrl(), url($redirect, array('absolute' => TRUE)), t('Redirected from %request to %redirect.', array('%request' => $request, '%redirect' => $redirect)));
}
?>
The code $this->getUrl() still has clean URLs enabled, while the code that runs with the $this->drupalGet() does not have clean URLs enabled. Before patch, tests fail, after patch, tests pass. Should be a clean case to get this in since it was already accepted for D7 and this does not break any APIs.
#8
#9
The D6 patch looks identical to the patch for HEAD, and it appears to be working.
#10
Yeah it's been working perfectly on my 6.x install.
#11
Thanks, committed to Drupal 6 as well.
#12
Automatically closed -- issue fixed for 2 weeks with no activity.