Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Test to watch: StatisticsLoggingTestCase->testLogging()
http://qa.drupal.org/pifr/test/194693
http://qa.drupal.org/admin/reports/event/4180283
Comment | File | Size | Author |
---|---|---|---|
#30 | 1346772-30_StatisticsLoggingTestCase_long_path.patch | 1.11 KB | scor |
#13 | 1346772-13.patch | 616 bytes | xjm |
#12 | statistics-1346772-12.patch | 1.89 KB | xjm |
#9 | statistics-path-no-whitespace.patch | 871 bytes | xjm |
#8 | statistics-path.patch | 954 bytes | xjm |
Comments
Comment #1
xjmSetting postponed for now. I think I saw a different failure in the statistics module the other day but I've not tracked it down yet. If it turns out the test is bugged then we can move this to
statistics.module
.Comment #2
jthorson CreditAttribution: jthorson commentedSimular statistics test failure seen on http://drupal.org/node/1290710 (though you may not see it, since it's being retested) ... appears to be a intermittent problem with that particular test.
Comment #3
penyaskitoSame issue in #1325488-10: Add an option to show help text before field
Comment #4
xjmSo pretty clear this test needs fixing. Moving to
statistics.module
and bumping to major, because it's really annoying to have one out of every five or ten tests fail randomly. :PComment #5
xjmThe last line here is the one that fails (line 139 of
statistics.test
):First value from the original post above:
'o;;\\Vo `!V,(">O/|^zckPbK&ZNs1S=7"F)|q%Hdx5U47j||s&<G2]0Mx|A $\\D[q9~=!zzm$#{dS\\*=%_G3pHI]DV\\V1J|[9mmos[&x*Z/d,RVBeLE\\(va@C@Yg\\3z~/B$j+1Y"U<`\\$H/A@<<1+Df(M(!RC=6QHrF4BPNf^pq7ZGR<zwdac~cE-7o6v?{wH|T1(eDn6u3\\DZz=FGA/Ehkrr[g!/*lj6:CMxl1i_9.<mk? #3N4LI}/Bzh7\\!J'
Second value:
'/o;;\\Vo `!V,(">O/|^zckPbK&ZNs1S=7"F)|q%Hdx5U47j||s&<G2]0Mx|A $\\D[q9~=!zzm$#{dS\\*=%_G3pHI]DV\\V1J|[9mmos[&x*Z/d,RVBeLE\\(va@C@Yg\\3z~/B$j+1Y"U<`\\$H/A@<<1+Df(M(!RC=6QHrF4BPNf^pq7ZGR<zwdac~cE-7o6v?{wH|T1(eDn6u3\\DZz=FGA/Ehkrr[g!/*lj6:CMxl1i_9.<mk? #3N4LI}/Bzh7\\!'
I imagine maybe these aren't valid paths, so that would explain the failure?
Edit: The logged path (second value) has a slash added at the beginning, and the truncated original path (first value) ends in a J that gets bumped off when the slash is added. So something in
drupalGet()
is causing that slash to be prepended.Edit 2: The path contains
:
,?
, and#
, which could causeurl()
to alter it.Comment #6
xjmNote that this path, crazy as it is, appears to be a perfectly valid URL alias. At least, it does not fail validation.
Comment #7
xjmAha. I was mistaken before. The second value is actually the original, generated path, which happened to start with a
/
-- which, naturally, gets stripped. That would explain it. So, maybe we should always add one alphanumeric character to the beginning and end of the path to ensure it doesn't have leading or trailing slashes.I don't know if that's why the other tests failed, though. I can't imagine the initial character is a slash that often. A 1 in 128 chance or so?
Comment #8
xjmComment #9
xjmComment #10
catchWhy not just a longer $this->randomName() instead of the wrapping?
Nice work tracking this down!
Comment #11
xjm#10: Well, I was thinking that we should still test with all those non-alphanumeric characters, since they are valid path characters. I could go either way, though.
What would be totally awesome would be for someone to run this particular test case a few thousand times both with and without the patch to make sure the patch fixes the test. I'm assuming drush has testing integration one could use to script it. Edit: If it doesn't, then we should probably do as catch suggests, and additionally create a followup issue to test our path handling with non-alphanumeric characters.
Comment #12
xjmAlright, actually, there are other weird failures too when I run the test repeatedly. E.g., the path gets set to NULL:
So here's a patch that just uses
randomName()
as suggested in #10.Comment #13
xjmEr. Without random hunks from other stuff.
Comment #17
xjmNote that #12 actually may have been a problem with my local environment. Drupal was throwing a 403 instead of a 404 for any path string over 255 characters unless it included a fragment or query string, which meant that NULL was logged instead of the requested path. I can't reproduce that now, though.
Comment #18
xjmAlright, my script just finished running #13 1000 times with no failures. So, I think/hope that is the right fix.
Comment #19
xjmYeah, this test is in D7 too.
Comment #20
bfroehle CreditAttribution: bfroehle commentedYes, using randomName() here is the easiest way to remove these spurious errors.
Comment #21
webchickGreat. Committed and pushed to 8.x and 7.x. Yay for fewer random, frustrating fails, and yay for killing a major bug. :)
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedI'm seeing similar failures running these tests locally. The 403 I see comes from Apache, not Drupal, though; it is simply refusing to serve a path that's as long as the ones used in this test. It may be a server configuration issue, but if so, it's probably a pretty common one.
Interestingly, I only see this when running the tests in Drupal 7, not Drupal 8. The difference is that for some reason, the tests in Drupal 7 wind up running with clean URLs on, but in Drupal 8 they run with clean URLs off. Not sure if this change in Simpletest behavior is intentional or a bug, but either way, it causes the tests to fail for me locally only in Drupal 7 because this:
makes Apache unhappy, but this:
doesn't.
Reopening this but at lower priority (since the testbot itself apparently isn't affected).
Comment #23
sanduhrsI can confirm the behavior described in #22 on Ubuntu 11.04 Server (ext4).
Perhaps the issue described in http://serverfault.com/questions/120397/max-length-of-url-257-characters... is the reason for it?
Comment #24
xjmComment #25
Dave ReidI can confirm this as well.
Comment #26
scor CreditAttribution: scor commentedable to reproduce this with latest core (right before 7.13) on two machines:
- testbot instance: fresh debian that rfay built today PHP 5.3.3-7+squeeze6 with Suhosin-Patch (cli) (built: Jan 31 2012 10:56:51)
- my localhost is MAMP with PHP 5.3.6 (cli) (built: May 25 2011 20:42:15).
tested the cli via
php ./scripts/run-tests.sh --verbose --url http://localhost/d7-dev/ --color --class StatisticsLoggingTestCase
and on the web ui on localhost.Comment #27
xjmSo, if some hosts don't support paths longer than 255 characters, then why are we doing so? There's nothing functionally wrong with this test. Do we need to change how we fixed #1180642: PDOException in statistics_exit() when path longer than 255 characters?
Comment #28
webchickWhy is this major? Annoying, yes, but this is an edge case not worth holding up D8 features for.
Comment #29
xjmComment #30
scor CreditAttribution: scor commentedRunning the security patches on the private testbot today reminded me of this annoying issue again, so I looked into it some more. The testbed initialized by PIFR has clean urls disabled which is why public testbots don't encounter this issue, that's in line with what David descibed in #22.
When clean urls are enabled, Apache will try to match the path on the filesystem, and if the filename is > 255 it will 403. All we have to do is to break down the path with a '/'. This is actually already fixed in D8, the patch attached backports a part of #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support to D7.
Comment #31
xjmAwesome @scor. The fix makes sense, still provides the coverage intended, and is well-documented.
Comment #32
webchickConfirmed that this is the same fix as in 8.x. Committed and pushed to 7.x. Thanks!