| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | beejeebus |
| Status: | closed (fixed) |
| Issue tags: | Needs tests |
Issue Summary
After a new installation of Drupal 7 from scratch (after a CVS update), I am unable to upload a logo to customize the Garland theme, from the Garland theme config page (path: admin/appearance/settings/garland).
When I try to upload a custom logo, I get these errors:
* The specified file temporary://poplarmed.jpg could not be copied, because the destination garland_logo.jpg is invalid. This is often caused by improper use of file_unmanaged_copy() or a missing stream wrapper.
* The specified file temporary://poplarmed.jpg could not be copied, because the destination garland_logo.jpg is not properly configured. This is often caused by a problem with file or directory permissions.
I'm not sure where it's trying to upload the file... My files directory is apache-writeable (and I didn't get any errors during install; the status report also reports that the file system is fine/writeable).
Comments
#1
Just ran into this myself, and it's true for any theme. It looks like the theme settings system didn't get updated for stream wrappers. I'll look into it.
#2
Wow, I had no idea there were still parts of core that ignore FAPI and manipulate $_POST directly. I wonder what else grep would turn up.
This patch adds a validator to handle file uploads correctly. It also updates the submit function to work with stream wrappers, while still allowing admins to specify a path directly to their logo file in case they have filesystem access. Includes a basic test.
#3
Got a guru meditation error on that last post, and now I'm getting "uploads disabled". I promise there really is a patch! I'll try again later.
#4
Setting status back to avoid confusing me into thinking there's something to review, until there really is a patch. :)
#5
subscribe
#6
Sorry Jennifer! Here's an actual patch this time. :)
#7
The last submitted patch failed testing.
#8
The logo upload test passes when you run it from the browser, but fails when you run it from run-tests.sh. I don't have what it takes to debug the test system right now. I'm stripping out the tests and reposting the patch.
#9
i tested the patch, and uploading a new logo or favicon worked just fine, nice work.
i'm not sure about this in the submit handler:
<?php+ // If the user entered a path relative to the system files directory for
+ // a logo or favicon, store a public:// URI so the theme system can handle it.
+ if (!empty($values['logo_path'])) {
+ $values['logo_path'] = _system_theme_settings_validate_path($values['logo_path']);
+ }
+ if (!empty($values['favicon_path'])) {
+ $values['favicon_path'] = _system_theme_settings_validate_path($values['favicon_path']);
+ }
+
?>
feels like _system_theme_settings_validate_path() should return true or false, not a potentially changed $path or false. is there nothing in the file API to do the conversion you need for the theme layer? if there isn't, there probably should be one.
#10
I couldn't find anything in the file API to do what I wanted (if I missed something let me know). If we weren't in freeze I might have added it. But actually, thinking about it again, I'm not sure that would even be wise. The tricky bit of this patch is getting the same theme setting to work for both a "normal" path (i.e. misc/whatever.gif) and a stream wrapper URI, which is a messy way to handle files anyway. I'm not sure we want to encourage it by writing an API function for it.
#11
I downloaded the patch and applied it, it works for me!
Since ksenzee couldn't find anything in the file API to answer the comment by justinrandell - I'm changing the status to RTBC.
Cheers!
#12
Subscribing- problem's still there on fresh D7 install and this patch fixed it.
#13
IMO we need tests for this so we don't break it again.
#14
Could this be included with the tests at #519284: Add back tests for file transfer (if those get in) since we're still looking to test whether file transfers are failing?
#15
Setting this as critical, since this is a pretty basic need for a site.
#16
Do we know whether the testbot failure in #6 was real or not? I will request a retest just to see.
#17
jhodgdon requested that failed test be re-tested.
#18
I tested that the error is there. I applied the patch and it worked fine. I haven't reviewed the code closely, but nothing jumped out at me when I reviewed the code. A close review is probably the only thing missing to have this RTBC'd. And agree with the critical priority!
#19
As webchick noted, this needs tests before getting committed.
#20
resurrected the test from #6, updated to chase HEAD, lets see what the bot thinks.
#21
The last submitted patch, , failed testing.
#22
Re-test of from comment #2391402 was requested by @user.
#23
The last submitted patch, , failed testing.
#24
It seems as though (a) the testing system has some bugs in its text, such as #22 "Re-test of from..." and #23 "The last submitted patch, , failed testing". :)
It also seems as though the test FileFieldValidateTestCase is unlikely to have broken due to this patch (that is what the current test results http://qa.drupal.org/pifr/test/22976 are reporting).
So probably the testing system is broken again?
#25
jhodgdon: yep, and even better, that's not the test that fails locally for me with this patch.
i suspect there's a bug in simpletests setup as well, we don't seem to create a temporary files directory.
#26
something is very broken with the simpletest file directory setup code. with this change to file_prepare_directory:
<?php// Check if directory exists.
if (!is_dir($directory)) {
// Let mkdir() recursively create directories and use the default directory
// permissions.
if (($options & FILE_CREATE_DIRECTORY)) {
// Don't squash the errors from drupal_mkdir.
if (drupal_mkdir($directory, NULL, TRUE)) {
return drupal_chmod($directory);
}
else {
// We end up here for every single DrupalWebTestCase::setUp...
file_put_contents('/path/to/your/log/file', "Failed to create $directory\n", FILE_APPEND);
}
}
return FALSE;
}
?>
am i going nuts? can anyone else reproduce this?
#27
I did a clean install and ran the FileFieldValidateTestCase (the one that is reported as failing -- it is called "File field validation tests" in the Testing UI). It ran fine.
I then installed this patch and re-ran the test. It still ran fine.
I also verified that I can upload a logo (i.e. the patch fixes the reported problem).
So I'll reset this to RTBC, and hopefully the testing system will agree at some point...
#28
Ah, I didn't see justinrandall's comments before I submitted mine...
justinrandall: I'm reading from what you say that you think there is a bug in the simpletest module... in which case, maybe we should open a separate issue? But I think the current patch for the logo upload problem is indeed RTBC...
#29
woo, actually the fail i pointed to in #26 is caused by drush. i have a habit of running
drush install simpletestwhich copies simpletests' files into the files directory *with the ownership of the user running the drush process*. if this is not the web server user, as was the case for me, then all simpletest file operations fail.
the new test in this patch now passes locally and all is well. i'm reuploading and setting to 'needs review' to get one more testbot run. please set it back to RTBC if the bot comes back green.
#30
woo, all green.
#31
Excellent! I reviewed this and nothing stuck out to me.
Committed to HEAD! Another critical bug bites the dust. :)
#32
Automatically closed -- issue fixed for 2 weeks with no activity.