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).

CommentFileSizeAuthor
#29 logo-upload.patch9.69 KBAnonymous (not verified)
#20 logo-upload.patch9.69 KBAnonymous (not verified)
#8 605892-logo-upload-8.patch8.18 KBksenzee
#6 605892-logo-upload.patch9.97 KBksenzee
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented
Component: Garland theme » theme system
Assigned: Unassigned » ksenzee

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.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented
Status: Active » Needs review

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.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented

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.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Active

Setting status back to avoid confusing me into thinking there's something to review, until there really is a patch. :)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

subscribe

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented
Status: Active » Needs review
FileSize
605892-logo-upload.patch9.97 KB

Sorry Jennifer! Here's an actual patch this time. :)

Status: Needs review » Needs work

The last submitted patch failed testing.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented
Status: Needs work » Needs review
FileSize
605892-logo-upload-8.patch8.18 KB

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

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:

+  // 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.

ksenzee’s picture

ksenzee
she/her
Location Seattle area
CreditAttribution: ksenzee commented
Assigned: ksenzee » Unassigned
Status: Needs work » Needs review

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.

dcor’s picture

dcor CreditAttribution: dcor commented
Status: Needs review » Reviewed & tested by the community

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!

Alex UA’s picture

Alex UA CreditAttribution: Alex UA commented

Subscribing- problem's still there on fresh D7 install and this patch fixed it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

IMO we need tests for this so we don't break it again.

Alex UA’s picture

Alex UA CreditAttribution: Alex UA commented

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?

Alex UA’s picture

Alex UA CreditAttribution: Alex UA commented
Priority: Normal » Critical

Setting this as critical, since this is a pretty basic need for a site.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Do we know whether the testbot failure in #6 was real or not? I will request a retest just to see.

Status: Needs work » Needs review

jhodgdon requested that failed test be re-tested.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

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!

Alex UA’s picture

Alex UA CreditAttribution: Alex UA commented
Status: Needs review » Needs work

As webchick noted, this needs tests before getting committed.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
logo-upload.patch9.69 KB

resurrected the test from #6, updated to chase HEAD, lets see what the bot thinks.

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

The last submitted patch, , failed testing.

Status: Needs work » Needs review

Re-test of from comment #2391402 was requested by @user.

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

The last submitted patch, , failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

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?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

something is very broken with the simpletest file directory setup code. with this change to file_prepare_directory:


  // 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?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

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...

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

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...

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned »
Status: Reviewed & tested by the community » Needs review
FileSize
logo-upload.patch9.69 KB

woo, actually the fail i pointed to in #26 is caused by drush. i have a habit of running

drush install simpletest

which 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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

woo, all green.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Excellent! I reviewed this and nothing stuck out to me.

Committed to HEAD! Another critical bug bites the dust. :)

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests

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

Issue tags: +Needs tests

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.