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.Seen this in the test bot a few times, but just got it locally as well.
This is the assertion which fails:
The unmunged (simpletest_lSa.php.txt) filename matches the original (simpletest_lSa_.php.txt)
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | file-munge-filename-1.patch | 1.5 KB | c960657 |
| #7 | file_name.patch | 664 bytes | catch |
| #7 | file_name_comment.patch | 1.68 KB | catch |
Comments
Comment #1
Dave ReidI have also seen this happen once or twice with the bot. It seems like a very random occurrence.
Comment #2
lilou CreditAttribution: lilou commentedexample : http://testing.drupal.org/pifr/file/1/theme-registry-fix-d7.patch
Comment #3
lilou CreditAttribution: lilou commentedDoc (http://api.drupal.org/api/function/file_munge_filename/7) says :
Comment #4
drewish CreditAttribution: drewish commentedsubscribing... it would be helpful to know which test specificialy is failing.
Comment #5
webchick@drewish: It's the munged vs unmunged comparison. See catch's initial post. Or do you need more info?
Comment #6
drewish CreditAttribution: drewish commentedwell the function name so i could pull it up without having to dig through the file... not a biggie but i was hoping to find it on api.drupal.org... but i guess it doesn't do classes...
my guess would be that it's doing a random file name and some percentage of the time the name conflicts with the expectation.
Comment #7
catchThis is impossible to reproduce reliably, but I'm seeing about 2-5 patches marked CNW by the test bot a day because of this. Discovered it's slightly quicker to delete the comment then hit retest than post a new comment and hit retest, but it's still annoying as hell and time-consuming to tidy up, and I just realised deleting those comments makes it look like less of a problem than it actually is.
We can change the random name to a fixed one and try that for a while. Or the second patch just comments out that line in the test.
I've switched the test bot off again for now since I keep seeing confused patch posters in the issue queue, hence marking this to RTBC without review so we can at least start it up again.
If we try the first patch for a while to see how it goes that's fine with me, but if not the second should work just fine pending a proper a fix.
Comment #8
catchHere's a recent one. http://drupal.org/node/370454#comment-1244635
Comment #9
webchickI committed the one with the TODO, because I have a distinct feeling that we haven't seen the last of this yet.
Marking active (needs more info) for now. Let's see if this helps. If so, please mark back to fixed, if not, please attach a new patch. :D
Comment #10
webchickAlso, testing bot now re-enabled.
Comment #11
webchickThis has been running now for quite a few hours, and seems to be okay so far. The only 'phantom' failure I've seen popping up is #371363: Figure out why "Field form tests" has exceptions sometimes.
Comment #12
sun.core CreditAttribution: sun.core commentedProper status.
Comment #13
drewish CreditAttribution: drewish commentedah, i don't think it's actually fixed, just disabled. i'll try to get on this since it should be super simple.
Comment #14
catchNo longer breaking the test bot, so back to normal for re-adding the test.
Comment #15
c960657 CreditAttribution: c960657 commentedThis restores the original test.
The problem was that randomName() sometimes returned a string ending with underscore, so that $this->name would become e.g. simpletest_lSa_.php.txt. file_unmunge_filename() simply converts "_." to ".", i.e. it strips the underscore that was part of the original filename. Today, randomName() only returns letters and digits, so the test should pass.
I guess we should fix file_unmunge_filename(), though. But as far as I can tell, we cannot do this in a completely bulletproof way. Assume you have two files, foo.bar.txt and foo.bar_.txt. If you munge their filenames and unmunge them again, they will both end up as foo.bar.txt. Admitted, this is probably not a very realistic example, so perhaps less will do.
However, before I begin to fix this issue, I would like to understand the purpose of these functions. It isn't clear to me exactly what security issue they are trying to solve. They were added back in 2006 with the CVS comment fixing stuff. Any ideas?
Comment #16
lilou CreditAttribution: lilou commentedComment #17
c960657 CreditAttribution: c960657 commentedI raised the question about the purpose of file_munge_filename() in #710640: Improve documentation for file_munge_filename().
Comment #18
ff1 CreditAttribution: ff1 commented#15: file-munge-filename-1.patch queued for re-testing.
Comment #19
ff1 CreditAttribution: ff1 commentedfile_munge_filename() is now fully documented thanks to #710640: Improve documentation for file_munge_filename().
randomName() problem should be fixed as explained in #15.
Tests pass for patch in #15.
So I can't see any reason not to reinstate this test.
Comment #20
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.