Custom logo and favicon functionality inanely tries to support absolute local file paths

Thanks. ref from #1209314: "Path to custom logo" needs to clarify and give some examples

previews and example URL broken

Relative paths work for for me in both D7 and D8. I don't understand this part of the OP:

[EDIT: Redacted after re-reading queue.] This still needs a UX fix at least.

TWO issues:
First:
1. Upload an exists image

Use mama3.jpg to reference a file in the public filesystem, public://mama3.jpg for a specific filesystem, or /sites/default/files/mama3.jpg for an arbitrary local file within this site.

2. change custom path to "/sites/default/files/mama3.jpg" and SAVE

Use logo.png to reference a file in the public filesystem, /sites/default/files/mama3.jpg for a specific filesystem, or //sites/default/files/mama3.jpg for an arbitrary local file within this site.

Can we change the texts to static strings, may like this:

Use public://logo.png to reference a file in the public filesystem. "custom_path/logo.png" for an arbitrary local file relative to the Drupal installation directory.

possible make it same to admin/config/media/file-system?

2nd:
When drupal installed on Sub-dir, eg. http://example.com/drupal

"/sites/default/files/mama3.jpg" refer to "http://example.com/sites/default/files/mama3.jpg"

same @#14 errors happened.

EDIT:
doesn't it suppport '../filename.png' ??

@sun,

You right. Working now :). The latest stuff I seen is drupal will produce an ugly URL:
<img src="http://192.168.56.108/drupal8x/../test2/logo.png" alt="Home" />

+++ b/core/modules/simpletest/drupal_web_test_case.phpundefined
@@ -3138,8 +3138,21 @@ class DrupalWebTestCase extends DrupalTestCase {
-  protected function assertFieldByName($name, $value = '', $message = '') {
-    return $this->assertFieldByXPath($this->constructFieldXpath('name', $name), $value, $message ? $message : t('Found field by name @name', array('@name' => $name)), t('Browser'));
+  protected function assertFieldByName($name, $value = NULL, $message = NULL) {
+    if (!isset($message)) {
+      if (!isset($value)) {
+        $message = t('Found field with name @name', array(
+          '@name' => var_export($name, TRUE),
+        ));
+      }
+      else {
+        $message = t('Found field with name @name and value @value', array(
+          '@name' => var_export($name, TRUE),
+          '@value' => var_export($value, TRUE),
+        ));
+      }
+    }
+    return $this->assertFieldByXPath($this->constructFieldXpath('name', $name), $value, $message, t('Browser'));

I asked sun about this bit. This change seems somewhat out of scope to me. The assertion is used once in testThemeSettings(). It's a fairly simple cleanup and I'm not opposed to it being added here or anything, but I thought I'd point it out.

Changing the theme tests from standard/bartik to testing/stark also isn't part of the scope in the title, but sun explained that it's what exposes the bug and makes it testable.

+++ b/core/modules/system/system.admin.incundefined
@@ -530,6 +513,40 @@ function system_theme_settings($form, &$form_state, $key = '') {
+      $element['#description'] = t('Use <code>@implicit-public-file</ code> to reference a file in the public filesystem, <code>@explicit-file</ code> for a specific filesystem, or <code>@local-file</ code> for an arbitrary local file within this site.', array(

Hmmm, "An arbitrary local file within the site" seems pretty confusing. What does it mean?

+++ b/core/modules/system/system.admin.incundefined
@@ -672,6 +696,11 @@ function _system_theme_settings_validate_path($path) {
+  // Exclude unnecessary elements before saving.
+  form_state_values_clean($form_state);
+  $key = $form_state['values']['var'];
+  unset($form_state['values']['var']);
+
   $values = $form_state['values'];
 
   // If the user uploaded a new logo or favicon, save it to a permanent location
@@ -703,10 +732,7 @@ function system_theme_settings_submit($form, &$form_state) {

@@ -703,10 +732,7 @@ function system_theme_settings_submit($form, &$form_state) {
   if (empty($values['default_favicon']) && !empty($values['favicon_path'])) {
     $values['favicon_mimetype'] = file_get_mimetype($values['favicon_path']);
   }
-  $key = $values['var'];
 
-  // Exclude unnecessary elements before saving.
-  unset($values['var'], $values['submit'], $values['reset'], $values['form_id'], $values['op'], $values['form_build_id'], $values['form_token']);

Another unrelated cleanup?

#31: drupal8.system-theme-settings.31.patch queued for re-testing.

Looks good to me. The string changes are nice clarifications, but they'll have to come out for the D7 patch.

Yes this looks good to me too. Committed/pushed to 8.x, moving to 7.x for backport.

I'll pull out the string changes for a backport.

Removing the bits to change the #title and #description, but leaving the rest.

This is what happens when people don't include the final serial comma in multi-line arrays. :P

There was a whole section of the test for the D8 strings, because they have dynamic examples.
Removing that, and also removing the 'core/' prefix on the files, made it all pass locally for me.

Also, removing the hunks about switching the default theme, since #1181776: Change theme_default variable to Stark was D8 only.

Yeah, so that's actually a Drupal 7 patch, huh? Thanks Tim!

I was concerned that there are fewer test failures than in #16, but I double-checked and (of course) these were related to the dynamic strings (as stated in #44).

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

I added the before/after screenshots to the summary, listed the string changes, and tried to clarify the current status of the patch.

Document string changes

really awesome work on this fix. i think the string breakage is acceptable and agree it's part of the fix.

committed and pushed to 7.x. thanks!

also tagging for release notes

If the string changes are part of the fix, then we need a 7.x patch for the string changes.

@pillarsdotnet is correct. Bumping to major since we'll want that followup ASAP. The task is to create a backport more like #31. It might be easiest to revert this commit and apply the complete fix after.

Here are the changes.
I got a weird failure locally that I hope doesn't fail here. The same assertion works in D8.

Damn, those are the failures I saw locally. For some reason path_to_theme() is picking up 'seven' and not 'stark' in D7 but not in D8.

Should I roll this back?

Ok, I'm up to speed now. The patch I committed was the stripped down version, without the string fixes to make it more palatable for backport.

Since the revised patch adds / changes a lot of strings, I'd prefer to hold it to the beginning of next release to give translators a bit more time to catch up.

And since it's just string fixes, I think we can degrade to normal, too. But happy to discuss it more. I'd really prefer not to change the code base at this point if at all possible, in order to give folks sufficient time to test a non-moving target.

I would agree with this not being major. As sun explained, the string changes are part of the fix in that they convey that a fix happened at all. But it can definitely wait til after 7.13.

Here's an update to the patch in #55 which is the D7 version.

There were 2 reasons for the previous fails.

1. The "human-friendly values and form element descriptions for the logo" were not getting set correctly for a relative file path.

2. In the test, the relative file paths were never getting a $local_file reset, and were therefore always looking to equal the default $local_file we had set of themes/stark/logo.png. I added a fix for this but there may be a better way to go about it than what I've done.

Also I attempted to add some of the additional changes from the D8 patch as suggested in #61 but this introduced more fails so I have not included them.

Please don't remove tags for historic reference.

+++ b/modules/system/system.admin.incundefined
@@ -539,6 +535,26 @@ function system_theme_settings($form, &$form_state, $key = '') {
+      elseif (!empty($original_path)) {
+        $local_file = $original_path;

This conditional isn't in D8.

+++ b/modules/system/system.testundefined
@@ -1677,6 +1677,40 @@ class SystemThemeFunctionalTest extends DrupalWebTestCase {
+      // Adjust for relative path not in public filesystem.
+      else {
+        $local_file = $input;

This is also not in D8.

Restoring tag/status

Also, I don't think you want this issue assigned to me anymore. :)

Marked #658386: Reduce repetitive wording on image and shortcut icon appearance settings as a duplicate.

Marked #822524: "Use the default shortcut icon" is ambiguous. as a duplicate.

Please, add to the descriptions a line noting that the file must be in place before trying to enter that path into Drupal, otherwise the test comes back with "invalid file path".

I was wrestling with my site installation trying to find a path that Drupal would accept, thinking "first I'll figure out what bizarre combination of files+sites will pass its test, THEN I'll move the file to that place on the local filesystem". Turns out that a probe for file existence is part of its test.

I suppose it's all fine and well for Drupal to try and save the user from themselves, but it would be even more useful to have explicit descriptions of what exactly we can type into those fields.

Updated issue summary.

Marked #756500: Remove form element descriptions that are equal to their titles at themes settings page as duplicate of this issue. It only pertained to the logo and favicon descriptions, which will be taken care of once the backport to Drupal 7 here is complete.