Problem/Motivation

Non-US-ASCII characters are stripped out of uploaded filenames. PHP's basename() does not properly support streams or filenames beginning with international characters.

Proposed resolution

Replaces all instances of basename() with wrapper function drupal_basename(). Revised function strips unsafe characters and preserves international characters; includes a check for directory separator characters.

New tests and test changes:

- create, use, save, upload, download, and copy files with international filenames
- generation of URL for file with international filename
- 404 while fetching nonexistant randomly named file including international characters
- correction to recombine chunked headers in DrupalWebTestCase::curlHeaderCallback()

Remaining tasks

Needs review and documentation for drupal_basename().

User interface changes

None.

API changes

Adds drupal_basename().

Original report by [OnkelTem]

Hi,

I'm using D5 and got much troubles yesterday with uploading files with non-latin characters in file names: all of them had been stripping. This behaviour reproduces on both my Debian Etch servers with PHP5 (but not with PHP4).

After investigations I found, that standard PHP functions basename() is in charge for stripping. The comments on php.net conforms this, basename() is not safe:

http://php.net/manual/en/function.basename.php

Passing UTF-8 string like '????.txt' to basename() results to 'txt', cutting off filename.

Looks like there are no appropriate solutions or workarounds and basename() simply should be replaced with alternative.

Files: 
CommentFileSizeAuthor
#135 blackout2.patch28.94 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 36,885 pass(es).
[ View ]
#133 blackout.patch28.42 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 36,885 pass(es).
[ View ]
#133 interdiff-between-D8.patch9 KBdroplet
#132 blackout.patch27.61 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 36,890 pass(es).
[ View ]
#132 interdiff-between-D8.patch8.39 KBdroplet
#127 chineseFile_1.jpg54.94 KBbiydng
#127 pageNotFound.jpg34.03 KBbiydng
#115 278425-drupal_basename-115.patch28.7 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 34,380 pass(es).
[ View ]
#115 interdiff.patch9.73 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#114 Screenshot at 2011-12-29 13:54:09.png35.84 KBrogical
#107 278425-drupal_basename12-d7.patch28.58 KBandypost
#106 278425-drupal_basename12.patch28.69 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 34,245 pass(es).
[ View ]
#104 278425-basename-not-locale-safe-104.patch1.86 KBrealityloop
FAILED: [[SimpleTest]]: [MySQL] 33,792 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#101 drupal_basename11-d7.8.patch29.18 KBOnkelTem
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_basename11-d7.8.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#90 278425-drupal_basename11-1.patch27.61 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 33,590 pass(es).
[ View ]
#87 278425-drupal_basename11-d7.patch28.11 KBandypost
#86 278425-drupal_basename11.patch27.54 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 29,445 pass(es).
[ View ]
#74 278425-drupal_basename10-d7.patch29.4 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 278425-drupal_basename10-d7.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#72 278425-drupal_basename9-d7.patch29.11 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 26,746 pass(es), 2 fail(s), and 1 exception(es).
[ View ]
#71 278425-drupal_basename8-d7.patch29.06 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 26,721 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#66 278425-drupal_basename7-d7.patch29.62 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 278425-drupal_basename7-d7_1.patch.
[ View ]
#62 278425-drupal_basename7-d7.patch29.56 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,477 pass(es).
[ View ]
#60 278425-drupal_basename7-d7.patch29.65 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,475 pass(es).
[ View ]
#58 278425-drupal_basename6debug-d7.patch31.88 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,375 pass(es), 30 fail(s), and 10 exception(es).
[ View ]
#56 278425-drupal_basename6-d7.patch29.94 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,429 pass(es), 30 fail(s), and 10 exception(es).
[ View ]
#55 278425-drupal_basename6-d7.patch29.94 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,420 pass(es), 30 fail(s), and 10 exception(es).
[ View ]
#49 278425-drupal_basename5-d7.patch27.66 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,196 pass(es).
[ View ]
#47 278425-drupal_basename4-d7.patch26.94 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,197 pass(es), 0 fail(s), and 2 exception(es).
[ View ]
#46 278425-drupal_basename3-d7.patch26.17 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,203 pass(es), 1 fail(s), and 2 exception(es).
[ View ]
#41 278425-drupal_basename2-d7.patch26.42 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 22,194 pass(es), 1 fail(s), and 2 exception(es).
[ View ]
#39 278425-drupal_basename-d7.patch22.21 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,180 pass(es).
[ View ]
#35 278425-basename-4.patch22.2 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 22,139 pass(es).
[ View ]
#28 basename-3.patch19.78 KBkotnik
FAILED: [[SimpleTest]]: [MySQL] 22,107 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#25 basename-2.patch29.33 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 20,895 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#23 basename-1.patch30.4 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 20,245 pass(es).
[ View ]
#17 drupal_6_basename.patch7.76 KBOnkelTem
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_6_basename.patch.
[ View ]
#9 file_278425.patch13.71 KBdrewish
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_278425.patch.
[ View ]
#5 drupal_basename.patch13.09 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_basename_0.patch.
[ View ]
#3 drupal_basename.patch13.05 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_basename.patch.
[ View ]
#2 basename_replacement_includes.patch3.5 KBOnkelTem
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch basename_replacement_includes.patch.
[ View ]
#2 basename_replacement_modules.patch2.99 KBOnkelTem
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch basename_replacement_modules.patch.
[ View ]

Comments

UPD.

basename() is locale dependent.

For example, this works, no stripping:

setlocale(LC_ALL, 'ru_RU.UTF-8');
$file->filename = trim(basename($_FILES["files"]["name"][$source]), '.');

This doesn't:

setlocale(LC_ALL, 'C');
$file->filename = trim(basename($_FILES["files"]["name"][$source]), '.');

StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch basename_replacement_modules.patch.
[ View ]
new3.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch basename_replacement_includes.patch.
[ View ]

chx ( http://drupal.org/user/9446 ) has created a replacement for basename() called drupal_basename().

Code is:

function drupal_basename($path,$prefix = '') {
$path = preg_replace('|^.+[\\/]|', '', $path);
if ($prefix) {
$path = preg_replace('|'. preg_quote($prefix) .'$|', '', $path);
}
return $path;
}

Function goes to file.inc (is it good)?

My first patch. Replaced all occurences of basename() in 'includes' and standard 'modules' dirs.

This patch is only for current Drupal 5.7 release!!!

Status:Active» Needs review
StatusFileSize
new13.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_basename.patch.
[ View ]

This is against HEAD with a little doxygen.

Status:Needs review» Needs work

Patch does not apply (patch: **** malformed patch at line 100: Index: includes/locale.inc), but both the diagnostic and the implementation looks sane.

Also: this is a perfect usecase of unit testing. So this shouldn't go in without a good test plan.

Status:Needs work» Needs review
StatusFileSize
new13.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_basename_0.patch.
[ View ]

Here is a better patch. Damien, can you try writing test cases? Stuff like \0 in file names... maybe make them consisting all of 256 bytes.

subscribing

For paths like "/a/" and "/a///", the proposed drupal_basename() returns the empty string, but PHP's basename() returns "a". Is this intentional?

really needs some documentation for the parameters of drupal_basename. we could just copy and paste from php's docs.

Title:Using basename() is not safeUsing basename() is not local safe
StatusFileSize
new13.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_278425.patch.
[ View ]

Here's a re-roll that fixes the failed hunk in scripts/run-tests.sh and adds some PHPDocs.

Title:Using basename() is not local safeUsing basename() is not locale safe

Now that file.test landed I guess we could write some tests for this...

Status:Needs review» Needs work

Yes, tests please. This looks like something relatively obscure which would not come up in the course of regular usage. Perfect for a test case.

Assigned:Unassigned» drewish
Status:Needs work» Postponed

i'll write some tests once #203204: Uploaded files have the permissions set to 600 gets committed

Status:Postponed» Needs work

i think i'd put the wrong issue in the above post... i don't see what the that has to do with this issue. probably one of the other file_test.module patches.

subscribing

subscribing

StatusFileSize
new7.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_6_basename.patch.
[ View ]

Patch for D6.13

subscribing

The early pasted and linked chx's safe baseneme function not fully compatible the PHP original basename. I wrote own UTF-8 safe basename function, that works without problem.

function basename_safe($path) {
        $path = rtrim($path,'/');
        $path = explode('/',$path);
        return end($path);
}

Assigned:drewish» Unassigned
Status:Needs work» Active

This should probably be combined with the new dirname() implementation in #701358: The file API presumes a hiearchical file storage.

Note that on Windows, dirname() also treats backslash as a directory separator.

Subscribing...

Status:Active» Needs review
StatusFileSize
new30.4 KB
PASSED: [[SimpleTest]]: [MySQL] 20,245 pass(es).
[ View ]

New shot.

#17: drupal_6_basename.patch queued for re-testing.

StatusFileSize
new29.33 KB
FAILED: [[SimpleTest]]: [MySQL] 20,895 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Reroll.

Status:Needs review» Needs work

The last submitted patch, basename-2.patch, failed testing.

StatusFileSize
new19.78 KB
FAILED: [[SimpleTest]]: [MySQL] 22,107 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Okay, here's reworked patch from #25 that applies to current HEAD.

Status:Needs work» Needs review

Marking for review.

Status:Needs review» Needs work

The last submitted patch, basename-3.patch, failed testing.

@andypost: no, we don't want to rely on the broken locale feature.

For the hell of it, I simply can't figure out why did the patch from #28 failed MimeType test.

Anybody got a clue?

@kotnik test falls because simpletest stream wrapper is not registered... strange. Broken function file_get_mimetype()
You are using file_uri_target() in drupal_basename() with none-existent schema (simpletest://)

Try to run File API - File mimetypes test locally
Call to a member function getTarget() on a non-object

EDIT: found strange simpletest_test_stream_wrappers() in simpletest.module

Status:Needs work» Needs review
StatusFileSize
new22.2 KB
PASSED: [[SimpleTest]]: [MySQL] 22,139 pass(es).
[ View ]

Changed function file_uri_target() to work with DrupalLocalStreamWrapper class because getTarget() method could not be implementation specific so we can use it's static implementation

Andypost, thanks for explanation and for fixing the patch.

Priority:Normal» Major

Suppose this major because drupal used in multilingual sites

Status:Needs review» Reviewed & tested by the community

+++ includes/file.inc 19 Jul 2010 06:13:51 -0000
@@ -1948,6 +1955,32 @@ function drupal_dirname($uri) {
+    $uri = preg_replace('@'. preg_quote($suffix) .'$@', '', $uri);

(minor) Wrong string concatenation spacing here.

Aside from that, this patch pretty much simply replaces every basename() with with drupal_basename() throughout core. Bonus points for @ingroup php_wrappers.

Thus, RTBC.

Powered by Dreditor.

StatusFileSize
new22.21 KB
PASSED: [[SimpleTest]]: [MySQL] 22,180 pass(es).
[ View ]

Re-roll against current HEAD with fixed string concatenation pointed in #38

Status:Reviewed & tested by the community» Needs work

Dang. webchick correctly asked for tests for drupal_basename().

Status:Needs work» Needs review
StatusFileSize
new26.42 KB
FAILED: [[SimpleTest]]: [MySQL] 22,194 pass(es), 1 fail(s), and 2 exception(es).
[ View ]

Added non-latin prefix for generated filename to FileTestCase::createFile()
This prefix contains spaces to make sure that all file-related tests work fine.

<?php
$filepath
= 'Файл для тестирования ' . $this->randomName(); // Russian, means 'file for testing'
?>

Also I change a FileSaveDataTest::testWithFilename() to use non-latin file name because this issue starts

<?php
$filename
= 'Текстовый файл.txt'; // Russian, 'text file.txt'
?>

Changed FileTokenReplaceTestCase::testFileTokenReplacement() to test non-latin names

Fixed RetrieveFileTestCase::testFileRetrieving() because non-latine names returned as urlencoded and this also related to this issue

This looks very good, but I don't understand this change:

<?php
function file_uri_target($uri) {
   if (
$scheme = file_uri_scheme($uri)) {
-    return
file_stream_wrapper_get_instance_by_scheme($scheme)->getTarget($uri);
+    if (
$wrapper = file_stream_wrapper_get_instance_by_uri($uri)) {
+      return
$wrapper->getTarget($uri);
+    }
+    else {
+     
// getTarget() is not implementation specific, so we can directly
+      // call it without an instance.
+      return DrupalLocalStreamWrapper::getTarget($uri);
+    }
   }
   return
FALSE;
}
?>

The initial code looks correct to me.

does this intersect with #858528: file_uri_target() purpose is unclear at all?

@Damien Tournoud, This is a same as file_get_mimetype()
So it's just fallback to base implementation if no wrapper-class defined

Should we make getTarget() is static method?

For example FileMimeTypeTest::testFileMimeTypeDetection() is using simpletest wrapper

<?php
 
public function testFileMimeTypeDetection() {
   
$prefix = 'simpletest://';
?>

Which has no implementation, so we should use Base class implementation

Status:Needs review» Needs work

The last submitted patch, 278425-drupal_basename2-d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.17 KB
FAILED: [[SimpleTest]]: [MySQL] 22,203 pass(es), 1 fail(s), and 2 exception(es).
[ View ]

After talking with DamZ on IRC we decide to change schema in testFileMimeTypeDetection() to 'public://' because 'simpletest://' has no implementation and file_uri_target() leave as is.

StatusFileSize
new26.94 KB
FAILED: [[SimpleTest]]: [MySQL] 22,197 pass(es), 0 fail(s), and 2 exception(es).
[ View ]

Fixed broken test, non-latin names should be encoded.

Status:Needs review» Needs work

The last submitted patch, 278425-drupal_basename4-d7.patch, failed testing.

StatusFileSize
new27.66 KB
PASSED: [[SimpleTest]]: [MySQL] 22,196 pass(es).
[ View ]

New strange headers appears $11 and $15 ' =?UTF-8?B?V0k=?=" '

array ( 0 => 'HTTP/1.1 200 OK ', 1 => 'Date: Thu, 29 Jul 2010 05:24:39 GMT ', 2 => 'Server: Apache/2.2.9 (Debian) PHP/5.2.13 ', 3 => 'X-Powered-By: PHP/5.2.13 ZendServer/5.0 ', 4 => 'Set-Cookie: ZDEDebuggerPresent=php,phtml,php3; path=/ ', 5 => 'Expires: Sun, 19 Nov 1978 05:00:00 GMT ', 6 => 'Last-Modified: Thu, 29 Jul 2010 05:24:39 +0000 ', 7 => 'Cache-Control: private ', 8 => 'ETag: "1280381079" ', 9 => 'Content-Length: 87 ', 10 => 'Content-Disposition: inline; filename="=?UTF-8?B?0KTQsNC50Lsg0LTQu9GPINGC0LXRgdGC0LjRgNC+0LLQsNC90LjRjyBtSmE3Nk4=?= ', 11 => ' =?UTF-8?B?V0k=?=" ', 12 => 'x-foo: Bar ', 13 => 'Vary: Accept-Encoding ', 14 => 'Content-Type: text/plain; name="=?UTF-8?B?0KTQsNC50Lsg0LTQu9GPINGC0LXRgdGC0LjRgNC+0LLQsNC90LjRjyBtSmE3Nk4=?= ', 15 => ' =?UTF-8?B?V0k=?=" ', 16 => ' ', )

So explode() alerts notice so I add ':' if header have no ':'

<?php
       
if (strpos($header, 'HTTP/') === 0) {
         
$name = ':status';
         
$value = $header;
        }
        else {
+          if (
strpos($header, ':') === FALSE) {
+           
$header .= ':';
+          }
           list(
$name, $value) = explode(':', $header, 2);
          
$name = strtolower($name);        }
?>

Status:Needs work» Needs review

Changing status

That's because the mime encoder is splitting the header in two lines :(

fyi; another one that doesn't work right is "drupal_realpath". Found that out via lots of tests to the test bot.
Look at the previous patches in here: http://drupal.org/node/818818#comment-3116590

We should find culprit of this headers
headers definition http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

This caused by mime_header_encode() which splits header on chunks but header function does not join them back

StatusFileSize
new29.94 KB
FAILED: [[SimpleTest]]: [MySQL] 22,420 pass(es), 30 fail(s), and 10 exception(es).
[ View ]

Finally fixed version.

- Fixed DrupalWebTestCase::curlHeaderCallback() to join chunks
- Added test to check filename transfered correctly via private download method (this one uses mime_header_decode to make sure that header encoded correctly)

StatusFileSize
new29.94 KB
FAILED: [[SimpleTest]]: [MySQL] 22,429 pass(es), 30 fail(s), and 10 exception(es).
[ View ]

Fixed tab character in DrupalWebTestCase::curlHeaderCallback()

Status:Needs review» Needs work

The last submitted patch, 278425-drupal_basename6-d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.88 KB
FAILED: [[SimpleTest]]: [MySQL] 22,375 pass(es), 30 fail(s), and 10 exception(es).
[ View ]

Same patch with debug

Status:Needs review» Needs work

The last submitted patch, 278425-drupal_basename6debug-d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.65 KB
PASSED: [[SimpleTest]]: [MySQL] 22,475 pass(es).
[ View ]

Suppose now should work, first header "HTTP/" should be excluded and end of headers detected with trim()

PS same patch for simpletest module #869408: curlHeaderCallback() should join headers if no colon found

Status:Needs review» Needs work

The criteria for a follow-up header is not "it doesn't contain ':'", but "it starts with at least one SP or HT".

Status:Needs work» Needs review
StatusFileSize
new29.56 KB
PASSED: [[SimpleTest]]: [MySQL] 22,477 pass(es).
[ View ]

Changed comment and condition

Status:Needs review» Needs work

Nice patch. Here's a few problems I found:

+ * Gets the filename from a given path.
+ *
+ * PHP's basename() does not properly support streams or filenames beginning
+ * with a non-US-ASCII character.
+ * @see http://bugs.php.net/bug.php?id=37738
+ *
+ * @see basename()
+ * @ingroup php_wrappers
+ */

This should probably read:

+ * Gets the filename from a given path.
+ *
+ * PHP's basename() does not properly support streams or filenames beginning
+ * with a non-US-ASCII character.
+ *
+ * @see http://bugs.php.net/bug.php?id=37738
+ * @see basename()
+ *
+ * @ingroup php_wrappers
+ */

// Headers starts with HTTP version and ends with empty line. They could be
+    // splited on chunks so they should be joined while been receiced. Details
+    // in rfc2616 http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Alternative:

// Headers start with an HTTP version and ends with an empty line. They could be
+    // split in chunks so they should be joined while being received. Details
+    // in rfc2616 http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

+      // Prefix with non-latin characters used to ensure that all file-related
+      // tests works with international filenames.

Alternative:

+      // Prefix with non-latin characters to ensure that all file-related
+      // tests work with international filenames.

+    // URLs could not contains characters outside the ASCII set so has to be
+    // encoded.

Alternative:

+    // URLs can't contain characters outside the ASCII set so $filename has to be
+    // encoded.

+    // Test the filename transfered correclly.

Alternative:

+    // Test that the file transfered correctly.

Status:Needs work» Needs review

dang I was hoping I'd get that in before you rolled another one.

Getting closer!

<?php
-    $this->headers[] = $header;
+   
// Header fields can be extended over multiple lines by preceding each
+    // extra line with at least one SP or HT. They should be joined on receive.
+    // Details are in RFC2616 section 4.
+    if ($header[0] == ' ' || $header[0] == "\t") {
+     
$this->headers[] = array_pop($this->headers) . $header;
+    }
+    else {
+     
$this->headers[] = $header;
+    }
?>

This needs to read array_pop($this->headers) . ' ' . trim($header); and the other should be trim($header) We need to normalize the whitespace here, because:

"Header fields can be extended over multiple lines by preceding each extra line with at least one SP or HT."

"Any LWS that occurs between field-content MAY be replaced with a single SP before interpreting the field value or forwarding the message downstream."

StatusFileSize
new29.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 278425-drupal_basename7-d7_1.patch.
[ View ]

@dhthwy, thanx a lot for wording!

@Damien Tournoud, Fixed, but not sure that we need a whitespace normalization here because mime_header_decode() cares about it.

#66: 278425-drupal_basename7-d7.patch queued for re-testing.

#66: 278425-drupal_basename7-d7.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 278425-drupal_basename7-d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.06 KB
FAILED: [[SimpleTest]]: [MySQL] 26,721 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Re-roll against HEAD

StatusFileSize
new29.11 KB
FAILED: [[SimpleTest]]: [MySQL] 26,746 pass(es), 2 fail(s), and 1 exception(es).
[ View ]

Another round, missed 2 hunks

Status:Needs review» Needs work

The last submitted patch, 278425-drupal_basename9-d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 278425-drupal_basename10-d7.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Changed a test testPrivateFileTransfer() to check the contents of file against checking headers.

Maybe better to raise this to critical bacause none-ascii file uploads still broken

The latest reroll no longer checks the Content-Disposition header. Is that intentional?

While debuging tests I found that this header is not send on test run. You can see this in #72 testing results

Subscribing

Status:Needs review» Reviewed & tested by the community

- upload non-latin filenames, worked
- download, worked
- display, worked
- unlink, worked

patch with lots hunks but succeeded apply, should it Re-roll ?

Probably this could go into 7.1

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

I have reopened http://bugs.php.net/bug.php?id=37738 but I do not hope for a quick resolution there and if there would be one it'd take years before we could use that PHP version (remember, we dropped to 5.2.4-ubuntu-patched as the minimal PHP from 5.2.5...) so yeah let's go with this version.

Status:Reviewed & tested by the community» Needs work

<?php
+  if (file_uri_scheme($uri)) {
+   
$uri = file_uri_target($uri);
+  }
?>

I don't see the point of this. Could we please remove it?

@Damien: The purpose is to make sure that drupal_basename('public://') returns '', not 'public:'.

I see. We could really use unit tests here.

Status:Needs work» Needs review
StatusFileSize
new27.54 KB
PASSED: [[SimpleTest]]: [MySQL] 29,445 pass(es).
[ View ]

Reroll against D8 with removal of trim suggested by Damien

@Damien Any ideas for unit tests? Default basename() implementation is not trimming scheme so fixed

<?php
  $file
= 'public://aa.txt';
  print
'basename:' . basename($file) . ".\n";
// returns - basename:aa.txt.
 
$file = 'public://';
  print
'basename:' . basename($file) . ".\n";
// returns - basename:public:.
?>

@chx Another duplicate of php bug http://bugs.php.net/bug.php?id=37268

StatusFileSize
new28.11 KB

Same patch for D7, additionally fixes hunk within user.install

#86 and #87 has 2 new hunks at modules/update/update.module

Any reviews? This bug is really annoying for me

Status:Needs review» Needs work

+++ b/includes/file.inc
@@ -2210,6 +2210,30 @@ function drupal_dirname($uri) {
+function drupal_basename($uri, $suffix = NULL) {
+  $separators = '/';
+  if (DIRECTORY_SEPARATOR != '/') {
+    $separators .= DIRECTORY_SEPARATOR;
+  }
+  $uri = rtrim($uri, $separators);
+  $uri = preg_match('@[^' . preg_quote($separators, '@') . ']+$@', $uri, $matches) ? $matches[0] : '';
+  if ($suffix) {
+    $uri = preg_replace('@' . preg_quote($suffix) . '$@', '', $uri);
+  }
+  return $uri;
+}

Took me some good minutes to figure out what is actually being done here, and why.

Can we add three inline comments, please? (one comment per atomic operation block; separator setup + trim, basename, suffix)

Also, the ternary preg_match() logic is extremely compact. Nice trick -- but when writing this as a regular if control structure, then we could skip the additional $suffix operation if $uri is '' empty anyway.

5 days to next Drupal core point release.

StatusFileSize
new27.61 KB
PASSED: [[SimpleTest]]: [MySQL] 33,590 pass(es).
[ View ]

reroll only.

Awesome! That PATCH is ok for my DRUPAL7, Thankyou!

But I have to say it is a very important BUG in DRUPAL, and It must be pay attention to.

Status:Needs work» Needs review

Marking needs review.

Tagging issues not yet using summary template.

#74: 278425-drupal_basename10-d7.patch queued for re-testing.

Status:Needs review» Needs work

needs docs and #89

It's another issue.

While your file system / OS isn't UTF-8, you will failed to upload a file with any UTF-8 characters.

eg.
After patched:
€ = failed.
€-abc.png = failed.

Before patched
€ = failed.
€-abc.png = okay. (renamed to -abc.png)

Probably it will got error on Windows.

I don't see any mention of http://drupal.org/project/transliteration on this issue, I've not used that module before but it exists specifically for this problem afaik.

transliteration converted filename to US-ASCII characters.

Without transliteration module we can't get actual filename if none-ascii letters are used in it. With transliteration we never get actual filename too :(

EDIT: actual means the original filename of file been uploaded.

This mostly usefull when filename is descriptive and has more than one word, without this patch I cant manage none-ascii filenames because core returns them trimmed to last word

subscribe

StatusFileSize
new29.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_basename11-d7.8.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Updated patch from #87 for current stable D7 release - 7.8. Two hunks was failing on some reason.

Added issue summary.

Thanx for Summary, @Marie Wendel

Patch needs re-roll for D8 'cos core now lives in core folder

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
FAILED: [[SimpleTest]]: [MySQL] 33,792 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

rerolled for /core

Status:Needs review» Needs work

The last submitted patch, 278425-basename-not-locale-safe-104.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.69 KB
PASSED: [[SimpleTest]]: [MySQL] 34,245 pass(es).
[ View ]

Added inline comments inside drupal_basename() (needs review and probably correction)

Added hunk for new image width test

PS: D7.9 patch also needs re-roll

Status:Needs review» Needs work
StatusFileSize
new28.58 KB

D7 re-roll

Status:Needs work» Needs review

Actually needs review

The patch is okay but as my comment #96, it still failed with non-UTF8 system. While upload is failed, we get following message:

File upload error. Could not move uploaded file.

For end users, they don't know why it can't move. I think we could suggest users to rename and try again.

Another way is to store filenames as hash but this will require to bootstrap core to retirn public files

While this patch looks OK (didnt review deeply so not RTBC'ing) the "If your file system / OS isn't UTF-8, uploading a file with any >127 UTF-8 characters fails." is a separate issue.

#106: 278425-drupal_basename12.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests

+ $filename = preg_replace('@' . preg_quote($suffix) . '$@', '', $filename);

should be preg_quote($suffix, '@'). There's a drupal_web_test_case.php hunk in the patch which looks good but a separate issue. And, it needs tests or discussion why we can't test this (locale dependency might stop testability).

StatusFileSize
new35.84 KB

patched in #107 tests well, test

can't wait to remove the transliterlation.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new9.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
new5.01 KB
PASSED: [[SimpleTest]]: [MySQL] 34,381 pass(es).
[ View ]
new28.7 KB
PASSED: [[SimpleTest]]: [MySQL] 34,380 pass(es).
[ View ]

reroll +@113

we already added non-latin tests.

+++ b/core/modules/simpletest/tests/file.testundefined
@@ -2220,11 +2222,14 @@ class FileSaveDataTest extends FileHookTestCase {
+    $filename = 'Текстовый файл.txt';
+++ b/core/modules/system/system.testundefined
@@ -2240,7 +2240,7 @@ class RetrieveFileTestCase extends DrupalWebTestCase {
+    $filename = 'Файл для тестирования ' . $this->randomName();

non-latin test

-1 days to next Drupal core point release.

I cancelled one interdiff but also showed. 2 interdiff are same but with diff format.

Status:Needs review» Needs work

The last submitted patch, interdiff.patch, failed testing.

Status:Needs work» Needs review

Version:8.x-dev» 7.10
Assigned:Unassigned» gghchem
Priority:Major» Critical

I am using windows xp sp3 and drupal 7.10,after patched the patch 278425-drupal_basename12-d7.patch,I find it's not works well.
sometimes I can upload a file and always i cannot upload the files with chinese names.It always shown:
Warning: move_uploaded_file(E:\xampp\htdocs\eee\sites\default\files\bee/北京市西城区2008—2009学年度初三年级第一学期期末测试化学试卷.doc) [function.move-uploaded-file]: failed to open stream: Invalid argument 在 drupal_move_uploaded_file() (行 1597 在 E:\xampp\htdocs\eee\includes\file.inc).
Warning: move_uploaded_file() [function.move-uploaded-file]: Unable to move 'E:\xampp\tmp\php6F.tmp' to 'E:\xampp\htdocs\eee\sites\default\files\bee/北京市西城区2008—2009学年度初三年级第一学期期末测试化学试卷.doc' 在 drupal_move_uploaded_file() (行 1597 在 E:\xampp\htdocs\eee\includes\file.inc).
文件上传错误。无法移动上传的文件。
I had asked so many pepole to help me,but the problem is still on.

Version:7.10» 8.x-dev
Assigned:gghchem» Unassigned
Priority:Critical» Major

1) You only should assign bugs to you when you're going to fix them
2) Please leave as major...
3) This needs to be fixed in D8 first, will be backported soon after

If you could install an empty D8 site and test the patch in #115 chances are bigger this will get commited to D7 soon.

Version:8.x-dev» 7.10
Priority:Major» Critical

see your path: E:\xampp\htdocs\eee\sites\default\files\bee/北京市西城区2008—2009学年度初三年级第一学期期末测试化学试卷.doc

this is not correct, but I just confused why it outputs an error path.

Version:7.10» 8.x-dev

sorry, this is an cocurrent posting.

Ow gghchem you have problems because your temporary folder is incorrect. You should change it in the file system settings to temp or something like that (without '/').

Priority:Critical» Major

Still not critical...

Read from #96, actually it doesn't support Windows very well. (maybe Windows Sever works). anyone try to test with Mac OS ?

(It's a well-known PHP problem. PHP6 will has a better support of UTF8)

Version:8.x-dev» 7.10
Category:bug» support
Priority:Major» Critical
Issue tags:-API addition, -needs backport to D7
StatusFileSize
new34.03 KB
new54.94 KB

I am using windows 7 Home and drupal 7.10. and ran the patch 278425-drupal_basename-115.patch. When I tried to upload a file with Chinese file name to a node, the file name appeared on the screen and the one saved in database are all in correct chinese characters, but the actual file saved in the public file directory is gibberish: for example 智慧.pps instead of 智慧.pps. This caused the "Page not found" error when I view the node and clicked on the file attachment link. The link path has the correct url encoded path and Chinese character filename. See the first two screenshots attached.

I have a feeling it is my server or php module that is messing up the encoding of the filename. I am using PHP 5.2.17 and apache 2.2 and I have set default charset to utf-8 in both. I noticed that when I opened the Apache online documentation, it is also in that weird encoding. Please help! Thanks!

Version:7.10» 8.x-dev
Category:support» bug
Priority:Critical» Major
Issue tags:+API addition, +needs backport to D7

#127, please read #121

stay away Windows. or save filename as transliteration/hash and a preprocess script to restore original file name during download. even if you use Linux, the latter is the best IMO.

我們先解決 Drupal 8,再修正 Drupal 7,不要變更主題狀態 :)
在 Windows 之下很容易出錯,盡量避免使用 Windows
或者自已寫一段程式,將檔案名以音譯或 HASH 保存,在下載時才換回正確檔案名 ( 像 Discuz 這類的處理方法 )

#113 code reviewed ?
#114 test reviewed. + myself.

ready to go ?

Status:Needs review» Reviewed & tested by the community

This looks good to me. The comments make sense, the function makes sense, and the replacements are good.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

OK read through this, code has had lots of review, most of the patch is find/replace and there's added test coverage for the bug.

Committed/pushed to 8.x., moving to 7.x for backport. This will need a change notice once it's been committed to 7.x (or someone could do that now too of course).

Status:Patch (to be ported)» Needs review
StatusFileSize
new8.39 KB
new27.61 KB
PASSED: [[SimpleTest]]: [MySQL] 36,890 pass(es).
[ View ]

StatusFileSize
new9 KB
new28.42 KB
PASSED: [[SimpleTest]]: [MySQL] 36,885 pass(es).
[ View ]

missed one file.

is not needed here?

user.install

$file->filename = basename($file->uri);

StatusFileSize
new28.94 KB
PASSED: [[SimpleTest]]: [MySQL] 36,885 pass(es).
[ View ]

I think all file management stuff are needed

Status:Needs review» Reviewed & tested by the community

Manually checked all places in D7 for #135 patch - except extended comments for introduced drupal_basename() and small fix for #113 it's the same as #107

we really need change notice which could be compiled from issue summary and http://bugs.php.net/bug.php?id=37738

Great to see patch commited before upcoming 7.11 release

Title:Using basename() is not locale safeChange notice for: Using basename() is not locale safe
Category:bug» task
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-needs backport to D7+Needs change record

Read through the patch and the comments here. Lots of good work. It's unfortunate we need to do an API change for this. :( But according to https://bugs.php.net/bug.php?id=37738, this is not getting fixed in PHP anytime soon.

Committed and pushed to 7.x.

We need a change notice for this, since this will affect contributed modules.

Title:Change notice for: Using basename() is not locale safeUsing basename() is not locale safe
Category:task» bug
Status:Active» Fixed

Assigned:Unassigned» nicxvan

Status:Fixed» Closed (fixed)

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

Is there a backport for D6?
If not, is it safe to simply copy the drupal_basename() function from the above patch and then change all uses of basename() in all files to drupal_basename()?

Priority:Critical» Major
Issue tags:-Needs change record

Assigned:nicxvan» Unassigned

Issue summary:View changes

Updated issue summary.