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.Locale module's po import is not safe mode ready. It tries to open and read the uploaded file straight from the PHP upload tmp dir, which is usually out of open_basedir on systems, where safe mode is turned on: ie. really cheap or free hosting accounts. The solution is to move_uploaded_file() the uploaded po to the Drupal temporary folder, import from there and remove the file. Since the file API is not yet move_uploaded_file() ready AFAIK, it needs to be made aware of this PHP restriction and provide a wrapper for this function, or locale module needs to use the PHP functions right away.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | Drupal-save-import-upload.diff | 3.75 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyHere is something to show what needs to be changed. Although I think this solution is not perfect, I have tried to signify earlier that I am not much experienced in the fileapi inner workings, so I am not the perfect person to submit this patch, but noone seems to get it up.
The patch changes
file_copy()to accept a move request, so that uploaded files, which are only about to be moved from the PHP upload folder (where they would get removed automatically later) can reuse the checking code infile_copy(). This makesfile_move()a wrapper aroundfile_copy(). Unsure if a$fileobject exclusively means that we deal with an uploaded file, I have added a newisuploadedfield, so that if a file is an uploaded one, we can perform a move_uploaded_file() operation, which is the only safe_mode compatible move we can do.copy()anunlink()does not work for files outside the open_basedir.I do think this patch needs heavy review, and I do think it should go into Drupal 4.5.1. Until this is fixed, people on systems with safe_mode are unable to upload and import a po file, thus they are unable to translate their website interface...
Comment #2
Steven CreditAttribution: Steven commentedThis patch means that you need to have the filesystem correctly configured to import a PO file. Not sure if this is a problem, but we should be aware of that.
I'm not sure we should keep file_move(). It's just a wrapper for file_copy(), I say get rid of it.
As far as $file->isuploaded goes, perhaps $file->uploaded would be better. We typically avoid varname prefixes like 'is' or 'not'.
About this:
+ $isuploaded = FALSE;
....
+ $isuploaded = isset($file->isuploaded) && $file->isuploaded;
Couldn't you just do away with all this? The 'isuploaded' member is only set to TRUE once, if it's unset then casting it to bool would be FALSE as well. In fact, I don't see why you need the $isuploaded variable at all. Just use if ($file->isuploaded).
Comment #3
Gábor HojtsyFirst I am unsure that I sent a clean solution. I also thought about moving out the checking logic from
file_copy(), and introducing afile_move_uploaded(), which would reuse that checking logic. That would keepfile_move()intact.Checking for
if ($file->isuploaded)is only safe if this member is always set. It could be, if every$fileobject comes from a file upload, but then this member is redundant... Otherwise you would get an E_NOTICE if that member is not set. My impression was that Drupal now tries to have clean code, which is E_NOTICE safe. I introduced the precomputed$isuploadto avoid using the expression I have used to compute it multiple times in the patch.Anyway, we need more people to review this.
Comment #4
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedDoes not apply anymore.
Comment #5
wulff CreditAttribution: wulff commentedStill applies. Uploading a .po file when
safe_modeis on andopen_basediris set results in the following errors:Comment #6
Gábor HojtsyContinued in the issue opened by Jose (long before this one).