Logo image settings form is broken, breaks per-theme overrides and can result in data loss

I also noticed this as of RC3 I think (at least the behavior described in the issue title title) but thought it was by design. I haven't encountered any issue with the logo not being rendered, though.

Was the logo disappear related to color in bartik? If so there is a patch here #2619332: Color scheme config changes aren't reflected in cached pages that is RTBC to resolve that.

@hatuhay it may still be related to cache like the one I mentioned. If you clear cache does the logo re-appear?

Oh sorry I miss-read the comments and didn't follow the issue summary through.

This may have happened around the time we moved the logo/branding variables into the block or maybe even earlier in some CMI change for overrides.

It would be ideal to know which commit this occurred on. RC3-4 would help narrow it down. If it was working in RC3 and not in RC4 you can use git bisect to narrow down which one.

@hatuhay Completely agree, we usually close the newer one (which is the other one) unless they were further ahead with a solution or more details on the problem.

Can we stop with the theatrics please? I'm sorry that I closed this issue in favor of the other one. I had already created that issue (after searching for an existing one with no luck) and uploaded a patch. Yes, this issue documents the side-effect and lengthy explanation for simply saying it no longer shows up. However this issue did not actually provide any helpful information about what or why the issue is actually occurring. It also didn't pinpoint the exact commit/issue/patch that caused this regression (#2005546-27: Use branding block in place of page template branding variables (site name, slogan, site logo)).

I seriously don't understand what you're so upset about. I'm not looking for "points" either, that's not what this is about. It's about actually providing a patch to fix the problem at hand. The ego and snarky remarks towards someone who is also just trying to help (with an actual patch) is rude and mean. This kind of behavior is what led to me leaving core dev and it's certainly not enticing me to come back anytime soon. Thanks...

---

Now, regarding this actual issue. The patch from the duplicate issue was apparently incomplete. Someone had a wonderful idea to duplicate the same array in \Drupal\Core\Extension\ThemeHandler::$defaultFeatures without removing the legacy function or adding any sort of documentation linking the two... fun how messed up the theme system (not twig) is, isn't it.

Here's a new patch that does fix the issue (with screenshots this time to prove it). Thanks for being so "supportive" to your fellow community member.

Before:

After:

@hatuhay I should have said @markcarver is likely not "looking for points" he probably figured he was further along in that other issue. Which is not uncommon. So I guess I didn't "completely agree". And seems to have got further before realizing the duplication.

@markcarver thank you for moving the patch to this issue and iterating on it.

Anyways, regarding the patch. One thing I noticed was in ThemeSettingsForm::buildForm() on line 140

    $toggles = array(
      'node_user_picture' => t('User pictures in posts'),
      'comment_user_picture' => t('User pictures in comments'),
      'comment_user_verification' => t('User verification status in comments'),
      'favicon' => t('Shortcut icon'),
    );

Should that also have logo in it? Considering 181 #states is looking for it?

        '#states' => array(
          // Hide the logo image settings fieldset when logo display is disabled.
          'invisible' => array(
            'input[name="toggle_logo"]' => array('checked' => FALSE),
          ),
        ),

I'm not sure just a guess based on reviewing what is changing in the patch.

According to the issue that took these out, no. The logo is now a block and if a user wishes to not use the logo, it can remove the block (or change which pages it shows up on). So the "toggle" makes no sense on the form anymore.

What that issue failed to realize though, is that it should be a theme that chooses to completely remove the logo feature as an "opt-in" instead changing the "defaults".

As far as the states on 181 goes, yes, that should probably come out. I'm attaching a new patch (no interdiff since it's so small).

FWIW, that entire form needs some serious touch-ups. My IDE lights up like already Christmas when I view it. I really don't have the energy for that though.

That's probably a good plan to remove #states, thanks. Fixing the whole form could get into scope creep land anyways, don't bother, we can put a follow-up for that.

Adding a tag for regression tests.

Assigning to myself for the tests.

Minor IS typo changes.

Bit of a simplistic test but it should suffice, though I noticed there is a test explicitly testing the opposite, I'm kinda curious if it will fail, but it didn't fail already so I wonder if it's testing anything...

\Drupal\system\Tests\System\ThemeTest:191

    $this->container->get('theme_handler')->install(array('bartik'));
    $this->drupalGet('admin/appearance/settings/bartik');
    // The logo field should only be present on the global theme settings form.
    $this->assertNoFieldByName('logo_path');
    $this->drupalPostForm(NULL, [], t('Save configuration'));

Put the wrong one up Mark informed me on IRC. Don't expect a difference in results.

Thanks for the heads up @hatuhay.

I removed that toggle_logo and also removed the seemingly useless test for the non-existence of the logo path field. Which either never gets called or is not testing the right thing.

Now that the field is showing up. Is it doing what is expected of it? Could someone manually test the theme override and maybe take a screenshot or two proof the logos are being used on the front end. So far our screenshots are of the admin form it would be nice to know that this feature still actually affects the front end logo.

Cool thanks, feel free to set the status to RTBC if you think we've sufficiently solved the problem @hatuhay.

I don't think this is critical.

Thanks all, I do think this is RTBC. The test code that's being removed I agree is not really testing anything at this point.

Can we get a follow-up please to remove or at least deprecate _system_default_theme_features()? It's only used in one place and we can add a getter to ThemeHandler instead IMO.

Not sure about removing the state, does that create a UX issue? Removing a progressive disclosure etc?

Critical because there can be loss of data/setting (from IS):

If a user saves the form without $form['logo'] present, it can potentially be removed from the theme settings config and cause a theme's logo to not be shown.

_system_default_theme_features() will have to be deprecated as there's at least one contrib release that I know of that uses it currently. I suppose an entirely separate issue could be created for that, but I don't think it would be too difficult to just do it here and now. My original patch failed because of this duplication of code, it'd be nice to get rid of that.

And no, removing the states doesn't create a UX issue. If anything, it ensures there isn't a future UX issue in case something changed in the States API that suddenly caused it to hide (not collapse) the details for logos because "toggle_logo" doesn't exist. That toggle was originally taken out in #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) and there shouldn't be any states on that form element for a non-existent toggle.

IMO let's keep the scope as is, especially if you consider it critical. I still don't.

This is not data loss in the sense that we mean for "critical".

(Rhetorical) So a site's logo disappearing isn't a big deal?

It has been abundantly clear that the theme system (not twig) was greatly belittled and overlooked in core.

(Sarcasm) Of course it makes sense any of its data loss would be as well.

I have no reliable "steps to reproduce" (mostly because I don't have the time to chase down the randomness of this), but I too have encountered this phenomenon (like @hatuhay) where the logo is simply "gone" after a multitude of cache rebuilding/theme setting saving (which happens when you're the one actually [re-]building a theme).

Whatever, I really don't have the time or desire to squabble over stupid and petty semantics.

I will gladly "demote" the priority of the issue if it can let people just focus on getting the flipping issue fixed.

Agreed on critical if the logo is disappearing.

Also removing [regression] from the title, regressions are neither worse nor better than other kinds of bugs.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!