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 |
clean-url-1.patch | 2.08 KB | c960657 | |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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 commentedI 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 commentedI 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 commentedOk, 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 commentedCommitted to CVS HEAD. Thanks.
Comment #7
Dave ReidI'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:
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.
Comment #8
Dave ReidComment #9
c960657 CreditAttribution: c960657 commentedThe D6 patch looks identical to the patch for HEAD, and it appears to be working.
Comment #10
Dave ReidYeah it's been working perfectly on my 6.x install.
Comment #11
Gábor HojtsyThanks, committed to Drupal 6 as well.
Comment #13
catchThis should be rolled back, see #523284: Optimize url().