Regression: Test failures with clean URLs

Posted by c960657 on January 9, 2009 at 10:46pm

Issue Summary

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	Status	Test result	Operations
clean-url-1.patch	2.08 KB	Idle	Passed: 8504 passes, 0 fails, 0 exceptions	View details

Comments

#1

Posted by Damien Tournoud on January 9, 2009 at 10:51pm

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

Posted by Dries on January 10, 2009 at 11:00am

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

Posted by c960657 on January 10, 2009 at 11:03am

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

Posted by Damien Tournoud on January 10, 2009 at 11:17am

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

Posted by Dries on January 10, 2009 at 11:50am

Status:

reviewed & tested by the community

» fixed

Committed to CVS HEAD. Thanks.

#6

Posted by System Message on January 24, 2009 at 12:00pm

Status:

fixed

» closed (fixed)

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

#7

Posted by Dave Reid on January 28, 2009 at 9:59pm

Version:	7.x-dev	» 6.x-dev
Priority:	critical	» normal
Assigned to:	c960657	» Anonymous
Status:	closed (fixed)	» 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.