Regression: Test failures with clean URLs [#356721]

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.

Comment	File	Size	Author
#8	356721-url-clean-urls-unstatic-D6.patch	1.38 KB	Dave Reid
#8
	clean-url-1.patch	2.08 KB	c960657

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 9 January 2009 at 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?

Comment #2

Dries CreditAttribution: Dries commented 10 January 2009 at 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.

Comment #3

c960657 CreditAttribution: c960657 commented 10 January 2009 at 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.

Comment #4

Damien Tournoud CreditAttribution: Damien Tournoud commented 10 January 2009 at 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.

Comment #5

Dries CreditAttribution: Dries commented 10 January 2009 at 11:50

Status:

Reviewed & tested by the community

» Fixed

Committed to CVS HEAD. Thanks.

Comment #6

24 January 2009 at 12:00

Status:

Fixed

» Closed (fixed)

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

Comment #7

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 28 January 2009 at 21:59

Version:	7.x-dev	» 6.x-dev
Assigned:	c960657	» Unassigned
Priority:	Critical	» Normal
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:

  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.