Regression: Test failures with clean URLs

c960657 - January 9, 2009 - 22:46
Project:Drupal
Version:6.x-dev
Component:base system
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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.

AttachmentSize
clean-url-1.patch2.08 KB
Testbed results
clean-url-1.patchpassedPassed: 8504 passes, 0 fails, 0 exceptions a href=http://testing.drupal.org/pifr/file/1/clean-url-1.patchDetailed results/a

#1

Damien Tournoud - January 9, 2009 - 22:51

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

Dries - January 10, 2009 - 11:00

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

c960657 - January 10, 2009 - 11:03

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_url during 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

Damien Tournoud - January 10, 2009 - 11:17
Status:needs review» reviewed & tested by the community

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

Dries - January 10, 2009 - 11:50
Status:reviewed & tested by the community» fixed

Committed to CVS HEAD. Thanks.

#6

System Message - January 24, 2009 - 12:00
Status:fixed» closed

Automatically closed -- issue fixed for two weeks with no activity.

#7

Dave Reid - January 28, 2009 - 21:59
Version:7.x-dev» 6.x-dev
Priority:critical» normal
Assigned to:c960657» Anonymous
Status:closed» needs review

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

Dave Reid - January 28, 2009 - 21:59

#9

c960657 - January 30, 2009 - 21:44
Status:needs review» reviewed & tested by the community

The D6 patch looks identical to the patch for HEAD, and it appears to be working.

#10

Dave Reid - January 30, 2009 - 22:26

Yeah it's been working perfectly on my 6.x install.

#11

Gábor Hojtsy - February 16, 2009 - 13:18
Status:reviewed & tested by the community» fixed

Thanks, committed to Drupal 6 as well.

#12

System Message - March 2, 2009 - 13:20
Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.

 
 

Drupal is a registered trademark of Dries Buytaert.