Theme's "Path to custom logo" always validated even if "Use the default logo" is set

Here's a rather "easy fix": Uncheck "Use the default logo", remove the custom logo path and then recheck "Use the default logo".

This is actually a Drupal core bug in system_theme_settings_validate() which always invokes _system_theme_settings_validate_path():
if (!empty($form_state['values']['logo_path'])) {

It should instead probably check:
if (!empty($form_state['values']['default_logo']) && !empty($form_state['values']['logo_path'])) {

We should also probably do the same fix for the custom favicon path as well.

Moving over to core IQ, should be checked against 8.0.x first and then backported to 7.x.

Thanks for the issue report. Here's one idea to resolve this. I just during validation check if the default checkbox is set and unset the value before the path is validated.

Give it a try and let us know if this will work for you.

Tested the above patch in Bartik. Confirmed it did not work beforehand and did work after the patch was applied. +1 RTBC

I think we want to add automated test coverage for this since this is a bug.

Confirmed that the patch is working in 8.2.0-Dev / Bartik

Re-adding tags.

@susannecoats - nice! useful to have confirmed the issue existed before applying the patch, and that the patch fixed it. I think I understood you plan(ned) to review the code for compliance with Drupal Code Standards. Is that a next step? (check this page for information about writing tests and other Contributor Tasks).

I am working on the "same fix for the custom favicon path" as listed in #1.

Added "default-favicon" update from the validation that was missing from last patch.

I think the test for this probably involves checking if the "Use the default logo supplied by the theme" or "Use the default shortcut icon supplied by the theme" is checked and looking at whether an error message was generated following submit. e.g.

If (box_checked and error) THEN fail; 
ELSE box_checked and no_error THEN pass;

However, I don't now how to check these things from a test, so someone else will need to implement this.

Thank you @tomatkins and @susannecoates for tackling this issue at the sprint yesterday.

@susannecoates I'll see if I can write up a similar test to your pseudo test code in #14. Thank you for thinking through the problem and confirming with me this approach seems to be on the right track as a solution to the problem.

My local testing is not quite working at the moment so to expediate I'm just going to expect this to work first time:)

I've got a test only patch that should fail, and a combined fix and test patch that should pass.

*crosses fingers*

Retested the patch because the test was a migrate random failure(has been happening all week randomly)

The change itself including tests looks good for me.

I also tested this manually by trying to save theme settings with invalid path for logo and favicon. The bug seems to still exist and it's fixed after this patch.

+++ b/core/modules/system/src/Form/ThemeSettingsForm.php
@@ -379,6 +379,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        $form_state->setValue('logo_path', '');
...
+        $form_state->setValue('favicon_path', '');

I think these should use unsetValue().

Thanks for teaching me we have a module API for that;)

Committed 429f46a and pushed to 8.1.x and 8.2.x. Thanks!

The new backport policy means that we should open a new issue for this against D7 if we want to fix it there.

Try not to mark issues fixed before the backport issue has been created. Otherwise the backport tends to get lost.

I went ahead and created #2741243: Theme's "Path to custom logo" always validated even if "Use the default logo" is set as a child issue now.