Color scheme config changes aren't reflected in cached pages

I was able to reproduce this - with some additional nuances.

1. Test 1: vanilla 8.0.0 system with the standard profile and Bartik theme (Page Caching disabled, CSS aggregation disabled)
   (see initial state image https://www.drupal.org/files/issues/2619332_Original-Color.jpg)
   1. Go to admin/appearance/settings/bartik and modify one or more of the theme's colors, then save the changes.
   2. Go to home page - see updated colors. (https://www.drupal.org/files/issues/2619332_Custom-Colors.jpg)
   3. Go back to admin/appearance/settings/bartik and reset the colors to the default color set "Blue Lagoon."
   4. Go back to home page - All colors are gone. (https://www.drupal.org/files/issues/2619332_No-Colors.jpg)
   5. Do a cache rebuild (drush cr) and colors return to original.
2. Test 2: vanilla 8.0.0 system with the standard profile and Bartik theme (Page Caching disabled, CSS aggregation enabled)
   1. Go to admin/appearance/settings/bartik and modify one or more of the theme's colors, then save the changes.
   2. Go to home page - see updated colors.
   3. Go back to admin/appearance/settings/bartik and reset the colors to the default color set "Blue Lagoon."
   4. Go back to home page - Colors remain as modified from step 1. (https://www.drupal.org/files/issues/2619332_Custom-Colors.jpg)
   5. Do a cache rebuild (drush cr) and colors return to original.

In testing further, I found that the site logo is also going away when changing theme colors.

I seem to have narrowed things down to color.module, in color_scheme_form_submit(), where colors.css, logo.svg, and the palette values in the config store are being deleted. (lines 418 - 433)

  // Delete old files.
  $files = $config->get('files');
  if (isset($files)) {
    foreach ($files as $file) {
      @drupal_unlink($file);
    }
  }
  if (isset($file) && $file = dirname($file)) {
    @drupal_rmdir($file);
  }

  // No change in color config, use the standard theme from color.inc.
  if (implode(',', color_get_palette($theme, TRUE)) == implode(',', $palette)) {
    $config->delete();
    return;
  }

I'll see if I can narrow down further and, hopefully, learn enough to craft a useful patch.

Thank you both, just changing the component.

I have a similar problem, except that the colors are not gone.
I can switch between all the preselections in the dropdown box without any problem, but when I select the default Blue Lagoon color, then the last selected color (example Ice) is shown.

Yes that seems to be the same issue I saw in comment #3 Test 2, when CSS aggregation is turned on.

Yes this quite reproducible thank you fro the steps in #3.

I tried to introduce some metadata to the branding block and page at least and that wouldn't work. (I'm an amatur cacheability metadata user)

Here's a patch... though it doesn't work, it show's the theory I was testing out.

This combo seemed to fix it for me. Could you two try this patch out?

Because I added new cache tags to the page, cache tag integration tests are why the failed test, easy fix, please test that it fixes your problem.

Yes - I verified that the patch in #9 resolves the issue for me.

Thanks for confirming, here's the extra cache tags in the integration test for the tests to pass.

1. +++ b/core/modules/color/color.module
   @@ -118,6 +118,7 @@ function color_block_view_system_branding_block_alter(array &$build, BlockPlugin
   +  $build['#cache']['tags'][] = 'config:color.theme.' . $theme_key;
    
      // Override logo.
      $logo = \Drupal::config('color.theme.' . $theme_key)->get('logo');
   

   You want to use the config object as a cacheable dependency. That way, you also get cache contexts/max age if applicable. (E.g. if someon has crazy use cases like changing the color based on the organic group or language or time of time).

   Untested code for that:
   $config = \Drupal::config(...);
   $cacheability_metadata = CacheableMetadata::createFromRenderArray($build);
   $cacheable_metadata->addCacheableDependency($config);
   $cacheable_metadata->applyTo($build);

   It's a bit convoluted but takes care of all the cache things and you don't have to figure out and hardcode cache tag names and things like that.

2. +++ b/core/modules/color/color.module
   @@ -429,6 +429,8 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
      if (implode(',', color_get_palette($theme, TRUE)) == implode(',', $palette)) {
        $config->delete();
   +    // Clear the library cache.
   +    Cache::invalidateTags(['library_info']);
   

   The problem with this is that it only works when you do a manual save in the UI and not when you e.g. deploy configuration.

   For that to work, you need to implement a Cache save (or in your case delete event). See Drupal\system\EventSubscriber\ConfigCacheTag for examples.

3. +++ b/core/modules/page_cache/src/Tests/PageCacheTagsIntegrationTest.php
   @@ -105,6 +105,7 @@ function testPageCacheTags() {
          'user:' . $author_1->id(),
          'config:filter.format.basic_html',
   +      'config:color.theme.bartik',
          'config:search.settings',
   

   We should probably some specific functional tests that assert that changing a setting actually updates the output?

Thanks @Berdir
#14.1 I can tackle unless someone wants to jump on it before this evening.

re #14.2 I'd like to do this, but may be out of my understanding at the moment. Though event subscribers sound cool.

Adding needs tests, tag for #14.3 And yes we should be able to reproduce this bug in a functional test, though could take a bit to get right as it did when I was manually testing.

I think this covers all 3 points from #14

Im not sure what the bug here is but will this also fix #2415663: Default color scheme selected returns early, fails to invalidateTags, if so can we mark the other one as a dupe and fix it here?

Nice, looks good to me.

Looks good :) An actual bug in the patch, and a bunch of nits.

1. +++ b/core/modules/color/color.module
   @@ -500,9 +503,6 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   -
   -  // Clear the library cache.
   -  Cache::invalidateTags(['library_info']);
   

   Why this change? Seems out of scope at best.

   I think this was removed by accident in response to @Berdir's review in #14.2. See the #16 interdiff.

2. +++ b/core/modules/color/src/Tests/ColorTest.php
   @@ -194,4 +194,43 @@ function testLogoSettingOverride() {
   +    // Login and set the scheme to slate.
   

   Nit: s/Login/Log in/
   Nit: s/scheme to slate/color scheme to 'slate'/

3. +++ b/core/modules/color/src/Tests/ColorTest.php
   @@ -194,4 +194,43 @@ function testLogoSettingOverride() {
   +    // Visit the bartik homepage and ensure color changes.
   

   Nit: s/bartik//

4. +++ b/core/modules/color/src/Tests/ColorTest.php
   @@ -194,4 +194,43 @@ function testLogoSettingOverride() {
   +    // Login and set the scheme back to default (delete config).
   

   Nit: s/Login/Log in/
   Nit: s/scheme/color scheme/

5. +++ b/core/modules/color/src/Tests/ColorTest.php
   @@ -194,4 +194,43 @@ function testLogoSettingOverride() {
   +    // Logout and ensure there is no color and we have the original logo.
   

   Nit: s/Logout/Log out/

1. Not really an accident. This issue added a second cache tag invalidation call to that function and I said that's not correct and should use a event subscriber. It's a bit scope creep but it just requires a single line in the new event subscriber (register for delete *and* save event) to make it work for both cases with the same code.

Completely on purpose to confirm Berdir's reply. It covers more cases and doesn't need duplicate code/calls. Cleaner way of dealing with config changes.

I will fix nits when I get to work in an hour. Thanks for reviews @Wim Leers and @Jeff burnz.

I see :) That makes sense!

One more thing that's kinda nitpicky, but less so than the ones in #20:

+++ b/core/modules/color/color.module
@@ -118,9 +118,13 @@ function color_block_view_system_branding_block_alter(array &$build, BlockPlugin
+  $cacheable_metadata = CacheableMetadata::createFromRenderArray($build);
+  $cacheable_metadata->addCacheableDependency($config);
+  $cacheable_metadata->applyTo($build);

This can also be simplified to:

CacheableMetadata::createFromRenderArray($build)
  ->addCacheableDependency($config)
  ->applyTo($build);

Nice, +1 to that clean up:)

I'm trying to get people from #2415663: Default color scheme selected returns early, fails to invalidateTags to comment on this issue. Please give credit to @netsensei and @chintan.vyas as well if they don't post here if possible.

Test nits have been picked from #20 and removed the variable with method chaining from #23

1. +++ b/core/modules/color/src/EventSubscriber/ConfigCacheTag.php
   @@ -0,0 +1,61 @@
   +class ConfigCacheTag implements EventSubscriberInterface {
   

   This class name is a bit strange. But no idea currently for a better one.

2. +++ b/core/modules/color/src/EventSubscriber/ConfigCacheTag.php
   @@ -0,0 +1,61 @@
   +    // Changing the color theme settings may mean a different CSS or JavaScript
   +    // library asset file.
   

   Comment is quite unclear.

   What about:
   Changing a theme's color settings causes the theme's asset library containing the color CSS file to be altered to use a different file.

There is an issue resolved https://www.drupal.org/node/2415663 related to #28 comment.

re #28.1
ConfigCacheTag -> ColorConfigCacheInvalidator?

#28.2 Sounds good to me, will fix in next patch

Name change and comment fix from #28

+++ b/core/modules/color/color.services.yml
@@ -0,0 +1,6 @@
+  color.config_cache_tag:

Should we change the id of the service also?

Otherwise RTBC!

Thanks nice catch. I still prefer CacheZapper but I'll settle;)

RTBC if this is green

Looks great :)

Awesome work, all!

Committer, please also give issue credit + commit credit to @jmarkel, his extensive research in #3 + #4 were instrumental.

Committed 66a3cc3 and pushed to 8.0.x and 8.1.x. Thanks!

+++ b/core/modules/color/color.module
@@ -193,7 +197,6 @@ function color_get_palette($theme, $default = FALSE) {
-  $base = drupal_get_path('module', 'color');

In future please try not to make unrelated and unnecessary changes in issues - it makes more work for reviewers.