Case-sensitivity of $default color scheme

This is a duplicate of #1196656: Mixed case hex values in default color scheme result in errors when selecting alternate color scheme..

Patch makes sense and is straight forward. However, please re-roll with the new Drupal::config() that's now in place.

tagging

Actually you should probably map to drupal_strtolower() instead, which is really just a wrapper around Unicode::strtolower(). Either should work.

This was a bit more than a reroll. Here's an updated patch.

Re-did the patch.

Confession: I hope I did this correctly, but not entirely sure.

The original code that the patch in #5 was trying to fix was as follows:

return $default ? $palette : variable_get('color_' . $theme . '_palette', $palette);

The suggested patch was:

return array_map('strtolower', $default ? $palette : variable_get('color_' . $theme . '_palette', $palette));

However, since that patch on July 29, 2012, the color.module was updated to the following line:

return config('color.' . $theme)->get('palette') ?: $palette;

Here's the approach I tried.

return Unicode::strtolower(Drupal::config('color.' . $theme))->get('palette') ?: $palette;

I changed the config() to Drupal::config() and wrapped that in Unicode::strtolower() as suggested in #7 and #9. I'm not sure if an array_map is what's needed here?

Anyhow, if somebody could take a look, that would be great. Thanks!

Hi @mdrummond :) Thanks for working on this issue and submitting patches!

I suspect this patch will fail as well. The errors from the first test indicates that strtolower needs the parameter to be a string and we're passing the CMI setting (which is the array). From reading the issue, the patch in #5 is attempting to use array_map() to lower case the values of the array (see: http://stackoverflow.com/questions/4445984/strtolower-on-a-array).

This being said, I'm not entirely what is being returned by the new CMI (ie: if it's the same simple array as before). Try doing this:

return array_map('drupal_strtolower', Drupal::config('color.' . $theme))->get('palette') ?: $palette);

My comment about using drupal_strtolower or Unicode::strtolower, was I'm not entirely sure what should be passed as the first parameter in array_map: array_map('drupal_strtolower', array_map('Unicode::strtolower'... or I think this would actually work as a valid callback: array_map(array('Unicode', 'strtolower')

Okay, let's see if this patch passes!

Triggering test bot

@@ -132,7 +132,8 @@ function color_get_palette($theme, $default = FALSE) {
+  return array_map('drupal_strtolower', Drupal::config('color.' . $theme))->get('palette') ?: $palette);

Ah! I see the problem. The array_map() function is getting closed too early. See the extra ) at the end of: Drupal::config('color.' . $theme))

One would think that eventually I would get this patch right. One will soon see.

#21: default-color-scheme-1415206-21.patch queued for re-testing.

+++ b/core/modules/color/color.module
@@ -132,7 +132,7 @@ function color_get_palette($theme, $default = FALSE) {
   // Load variable.
   // @todo Default color config should be moved to yaml in the theme.

One tiny nitpick (that I just noticed). This comment should probably be removed and replace with something says it loads the saved configuration palette or the default one if it doesn't exist. It also converts values to lowercase hexes. Follow documentation standards at https://drupal.org/node/1354#drupal and https://drupal.org/node/1354#inline.

Didn't need to re-test, it passed. I'm hesitant to tag as needs tests, this is a very minor change and can be covered with the above inline comment.

Added return statement with comments.

@NitinLama We cannot have a comment exceed 80 columns due to coding standards. It needs to wrap onto the next line.

Updated comment.

Nice work!!

Was reviewing as I went searching through D8.9.x, and it seems like the callback function "drupal_strtolower" only exist in D7's Unicode.inc file.

Back to need work if this patch is to be ported to D8.

Posting D7 version code for reference, might reconsider using mb_strtolower() as part of a callback function for array_map as an alternative to the Unicode::strtolower suggestion before since it's gonna be deprecated in D9.

/**
 * Lowercase a UTF-8 string.
 *
 * @param $text
 *   The string to run the operation on.
 *
 * @return string
 *   The string in lowercase.
 *
 * @ingroup php_wrappers
 */
function drupal_strtolower($text) {
  global $multibyte;
  if ($multibyte == UNICODE_MULTIBYTE) {
    return mb_strtolower($text);
  }
  else {
    // Use C-locale for ASCII-only lowercase
    $text = strtolower($text);
    // Case flip Latin-1 accented letters
    $text = preg_replace_callback('/\xC3[\x80-\x96\x98-\x9E]/', '_unicode_caseflip', $text);
    return $text;
  }
}

I have added the drupal_strtolower function definition in D8.9.x
Please review !
Thank you

Thanks for the effort on creating a new patch! Just a few things we need to check before moving forward.

The function drupal_strtolower($text) was deprecated and removed when upgrading from Drupal 7 to Drupal 8 for a reason, I don't think we should be adding it back into Unicode.inc file again. Let's just use "mb_strtolower" instead in the array_map function, this was what I saw changed in the _color_rewrite_stylesheet function comparing Drupal 7 to Drupal 8.

We still need to update the "issue summary" and do a "manual testing" to assure this patch is good before setting to "RTBC". Follow the guidelines on Issue summary and Manual testing for further information. ;) Thanks!

Going back checking on core's git history, the removal action of drupal_strtolower function happened in the following issues #2361825: Remove drupal_strtolower, #2361823: Remove usage of drupal_strtolower(), and see #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? for the policy reason for this deprecation change.

Drupal 8 color.module

/**
 * Rewrites the stylesheet to match the colors in the palette.
 */
function _color_rewrite_stylesheet($theme, &$info, &$paths, $palette, $style) {
  // Prepare color conversion table.
  $conversion = $palette;
  foreach ($conversion as $k => $v) {
    $v = mb_strtolower($v);
    $conversion[$k] = Color::normalizeHexLength($v);

Drupal 7

/**
 * Rewrites the stylesheet to match the colors in the palette.
 */
function _color_rewrite_stylesheet($theme, &$info, &$paths, $palette, $style) {
  $themes = list_themes();
  // Prepare color conversion table.
  $conversion = $palette;
  foreach ($conversion as $k => $v) {
    $conversion[$k] = drupal_strtolower($v);

Updated the mb_strtolower in array_map function as drupal_strtolower was removed from d8.
Please review

Patch still applies and is working as intended. Moving to RTBC.

1. +++ b/core/modules/color/color.module
   @@ -178,13 +178,10 @@ function color_get_palette($theme, $default = FALSE) {
   -  return \Drupal::configFactory()->getEditable('color.theme.' . $theme)->get('palette') ?: $palette;
   ...
   +  return array_map('mb_strtolower', Drupal::config('color.' . $theme)->get('palette') ?: $palette);
   

   We're changing behaviour here, previously it was color.theme.{theme} now it is just color.{theme}

2. +++ b/core/modules/color/color.module
   @@ -178,13 +178,10 @@ function color_get_palette($theme, $default = FALSE) {
   +  // It loads the saved configuration palette
   +  // Or the default one if it doesn't exist.
   +  // It also converts values to lowercase hexes.
   

   This comment is wrapped prematurely and we're losing a lot of the previous comment. Let's just add a sentence to the existing comment - something along the lines of.
   Ensure that hex values are cast to lowercase to ensure case-insensitivity.

Also, as this is a bug-fix, it would be good if we had some test-coverage that demonstrated the bug, and that the fix applied here resolves it.

Given the change made at item 1 didn't break any tests, I'm guessing there isn't much in the way of existing test-coverage for color module.

Color has been removed from core, #3270899: Remove Color module from core.