Posted by xjm

Problem/Motivation

The color module currently only saves a theme's color configuration to config when the color schema does not match any of the default color schemes provided by the theme, going out of its way to return early in color_scheme_form_submit() if the palette matches one of the default palettes. I'm not sure if this is due to it being in some weird interim limbo until #1712250: Convert theme settings to configuration system is in, but there doesn't seem to be any reason to do this, and the result is that you cannot rely on the configuration existing.

I discovered this when I tried to provide a local config override in a settings.php file:

$conf['color.bartik'] = array(
  'palette' => array(
    'top' => '#096d17',
    'bottom' => '#151e16',
  )
);

It worked fine until the main configuration got switched to a default bartik palette, whereupon the config file disappeared from disk, and as a result these two colors were the only ones loaded into the palette, so all the colors that were supposed to be inherited from the active configuration were lost.

Proposed resolution

The attached patch might be all that's needed to fix this--simply removing the weird exemption for the default color scheme.

Remaining tasks

This probably needs test coverage, and also some manual testing to make sure it works as intended. Just want to check the test suite before I go any further with it.

Files: 
CommentFileSizeAuthor
#11 color-1963922-3.patch1.08 KBmgifford
#3 color-1963922-3.patch1.08 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 54,130 pass(es). View
color.patch675 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Comments

xjm’s picture

Status: Active » Needs review
Issue tags: +Needs tests, +Needs manual testing
xjm’s picture

Issue tags: +Configuration system
xjm’s picture

FileSize
1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 54,130 pass(es). View

Looks like we still need the file to be created initially. I have no idea how CMI works with themes; does this work?

xjm’s picture

Title: Color module should save its configuration even when a predefined color scheme is used » Color module should save its configuration even when a predefined color scheme is used and regenerate files on synch
Assigned: xjm » Unassigned
Status: Needs review » Needs work

So, hm. #3 does result in a color palette being installed, but since all the stylesheet and image generation logic happens within the color module submit handler (and not on cache clear), one has to submit the form before any overrides specified in settings.php will take effect. Rewriting that is a bit more than I've bargained for here. :)

This also means that if you stage (e.g) color.bartik.yml, your theme is way broken until you submit the color form to regenerate the files locally on the site where you imported it.

Also, unrelated, the outside hue wheel of the color picker seems to be broken in Safari.

xjm’s picture

Title: Color module should save its configuration even when a predefined color scheme is used and regenerate files on synch » Color module should regenerate files on synch and save its configuration even when a predefined color scheme is used

I guess the next step is to factor out the file generation logic into something we can call during installation and config synch (maybe on cache clear?).

alexpott’s picture

#1890784: Refactor configuration import and sync functions adds the necessary symfony events to do this.

markcarver’s picture

I'll have to take a look at this more (in depth) when I have time.

xjm’s picture

Priority: Normal » Major

This actually has pretty serious implications for deploying color configuration, so bumping to major.

xjm’s picture

Issue summary: View changes

Removing myself from the author field so I can unfollow. --xjm

BTMash’s picture

Following up on this issue. It seems like a part of the issue was fixed (getting the palette in place). The color configuration itself seems to come over just fine. However, the part that doesn't work is where files need to be regenerated when moving from one environment to another (like local to stage or stage to prod / vice versa). These files are actually part of configuration so the yml file looks like:

palette:
  top: '#4c1c58'
  bottom: '#593662'
  bg: '#fffdf7'
  sidebar: '#edede7'
  sidebarborders: '#e7e7e7'
  footer: '#2c2c28'
  titleslogan: '#ffffff'
  text: '#301313'
  link: '#9d408d'
logo: 'public://color/bartik-b705ddc4/logo.png'
stylesheets:
  - 'public://color/bartik-b705ddc4/colors.css'
files:
  - 'public://color/bartik-b705ddc4/logo.png'
  - 'public://color/bartik-b705ddc4/colors.css'

Which is problematic since the other environment is not going to contain those files. Resaving the theme settings page brings everything up again correctly but you are then left with a difference in configuration. It seems like the logo, stylesheets, files should be in the key_value storage instead of CMI (and regenerate if they do not exist).

catch’s picture

Issue tags: +D8 upgrade path

This will need an upgrade path and probably configuration schema change, and yes key/value somewhere seems like the right place.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.08 KB

Just re-uploading patching #3 for the bots.

Status: Needs review » Needs work

The last submitted patch, 11: color-1963922-3.patch, failed testing.

heddn’s picture

This is related to #2675234: Color Module doesn't clear cache. This issue seems (based on its title) to only focus on predefined color schemes. But another part of the issue is that when the config is generated, the color scheme isn't used until you save a /form/. But if you use config_readonly, then you cannot save the admin form to generate the css/js/images. This is especially bad if you use config_installer to install the site. The site just looks bad and you cannot fix it until you disable readonly, save the color scheme, then re-enable readonly.

heddn’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.