Remove Bartik process layer functions, refactor color module alterations

taking on as part of Portland sprint

Removed process hooks from bartik.theme.
Removed _color_[html|page]_alter functions.
Added color_css_alter().

Added additional info about API changes

+++ b/core/modules/color/color.moduleundefined
@@ -77,31 +74,10 @@ function _color_html_alter(&$vars) {
-/**
- * Replaces the logo with a color-altered logo.
- *
- * A theme that supports the color module should call this function from its
- * THEME_process_page() function, so that the correct logo is included when
- * page.tpl.php is rendered.
- *
- * @see theme()
- */
-function _color_page_alter(&$vars) {
-  global $theme_key;
-
-  // Override logo.
-  $logo = config('color.' . $theme_key)->get('logo');
-  if ($logo && $vars['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $vars['logo'])) {
-    $vars['logo'] = file_create_url($logo);
   }

the given patch removes the color-altered logo functionality without any replacement.

i'll give this a try.

attached is a patch that extends the patch from #3 and introduces color_preprocess_page to retain the functionality formerly provided by _color_page_alter.

+++ b/core/themes/bartik/bartik.themeundefined
@@ -103,12 +84,7 @@ function bartik_preprocess_maintenance_page(&$variables) {
-}
 
-/**
- * Implements hook_process_HOOK() for maintenance-page.html.twig.
- */

actually this doesn't look good and appears to conflict with #1843710: Remove template_process_maintenance_page()

updating tags

sorry for the noise, actually #7 seems to be ok.

the attached patch integrates a bartik-related fix from #1843710-4: Remove template_process_maintenance_page() in order to get rid of patches conflicting each other.

added a fix related to #12

Tested @dasjo's patch and seemed to work normally! Traveling through the site there were no apparent changes to the bartik theme. In addition we checked the code and the process functions were gone.

Tested @dasjo's patch and it seems to work as desired. There were no apparent changes to the Bartik theme during common site use. The process functions were no longer in the code.

This patch is now blocking #2004286: Defer calls to drupal_get_* functions until printed inside a template by adding a RenderWrapper class

Added related issue

Retitling to reflect what this is doing, if anyone has a better title please update. From my understanding these changes need to be done at the same time otherwise things will be broken (I don't think the patch can be split in two). We are essentially changing the color module's API so Bartik needs to be updated accordingly. Tagging for change notification.

Reviewed the patch and it looks good to me, and tested the color module functionality manually as well.

Just tweaking the patch ever so slightly to not introduce an unneeded blank line before a docblock.

re-reviewed the patch (not that it was necessary, but just in case) and still +1.

Manually tested all looking good.

Committed aaa4e9d and pushed to 8.x. Thanks!

blocker

Assigning to self to write change notification...

Here's a draft of the change notice. Not sure about the length of the before/after code, would welcome comments on how/if it could be trimmed.

Summary

• Removed process layer functions from Bartik theme
• Color module doesn't require themes to call _color_html_alter and _color_page_alter anymore. These two functions have been removed, and themes that still use them will break.
• Hook implementations updated.
• Changed files: /core/modules/color/color.module and /core/themes/bartik/bartik.theme

Before:

<?php
 ...
/**
 * Replaces style sheets with color-altered style sheets.
 *
 * A theme that supports the color module should call this function from its
 * THEME_process_html() function, so that the correct style sheets are
 * included when html.tpl.php is rendered.
 *
 * @see theme()
 */
function _color_html_alter(&$vars) {
  global $theme_key;
  $themes = list_themes();

  // Override stylesheets.
  $color_paths = config('color.' . $theme_key)->get('stylesheets');
  if (!empty($color_paths)) {

    foreach ($themes[$theme_key]->stylesheets['all'] as $base_filename => $old_path) {
      // Loop over the path array with recolored CSS files to find matching
      // paths which could replace the non-recolored paths.
      foreach ($color_paths as $color_path) {
        // Color module currently requires unique file names to be used,
        // which allows us to compare different file paths.
        if (drupal_basename($old_path) == drupal_basename($color_path)) {
          // Replace the path to the new css file.
          // This keeps the order of the stylesheets intact.
          $vars['css'][drupal_basename($old_path)]['data'] = $color_path;
        }
      }
    }

    $vars['styles'] = drupal_get_css($vars['css']);
  }
}

/**
 * Replaces the logo with a color-altered logo.
 *
 * A theme that supports the color module should call this function from its
 * THEME_process_page() function, so that the correct logo is included when
 * page.tpl.php is rendered.
 *
 * @see theme()
 */
function _color_page_alter(&$vars) {
  global $theme_key;

  // Override logo.
  $logo = config('color.' . $theme_key)->get('logo');
  if ($logo && $vars['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $vars['logo'])) {
    $vars['logo'] = file_create_url($logo);
  }
}
...
?>

After:

<?php
...
/**
 * Implements hook_css_alter().
 *
 * Replaces style sheets with color-altered style sheets.
 */
function color_css_alter(&$css) {
  global $theme_key;
  $themes = list_themes();

  // Override stylesheets.
  $color_paths = config('color.' . $theme_key)->get('stylesheets');

  if (!empty($color_paths) && !empty($themes[$theme_key]->stylesheets['all'])) {

    foreach ($themes[$theme_key]->stylesheets['all'] as $base_filename => $old_path) {
      // Loop over the path array with recolored CSS files to find matching
      // paths which could replace the non-recolored paths.
      foreach ($color_paths as $color_path) {
        // Color module currently requires unique file names to be used,
        // which allows us to compare different file paths.
        if (drupal_basename($old_path) == drupal_basename($color_path)) {
          // Replace the path to the new css file.
          // This keeps the order of the stylesheets intact.
          $css[drupal_basename($old_path)]['data'] = $color_path;
        }
      }
    }
  }
}

/**
 * Implements hook_preprocess_page().
 *
 * Replace the logo with the colored version if available.
 */
function color_preprocess_page(&$vars) {
  global $theme_key;

  // Override logo.
  $logo = config('color.' . $theme_key)->get('logo');
  if ($logo && $vars['logo'] && preg_match('!' . $theme_key . '/logo.png$!', $vars['logo'])) {
    $vars['logo'] = file_create_url($logo);
  }
}
...
?>

Before:

<?php
...
/**
 * Implements hook_process_HOOK() for html.tpl.php.
 */
function bartik_process_html(&$variables) {
  // Hook into color.module.
  if (module_exists('color')) {
    _color_html_alter($variables);
  }
}

/**
 * Implements hook_preprocess_HOOK() for page.html.twig.
 */
function bartik_preprocess_page(&$variables) {
  // Pass the main menu and secondary menu to the template as render arrays.
  if (!empty($variables['main_menu'])) {
    $variables['main_menu']['#attributes']['id'] = 'main-menu-links';
    $variables['main_menu']['#attributes']['class'] = array('links', 'clearfix');
  }
  if (!empty($variables['secondary_menu'])) {
    $variables['secondary_menu']['#attributes']['id'] = 'secondary-menu-links';
  }
}

/**
 * Implements hook_process_HOOK() for page.html.twig.
 */
function bartik_process_page(&$variables) {
  $site_config = config('system.site');
  // Hook into color.module.
  if (module_exists('color')) {
    _color_page_alter($variables);
  }
  // Always print the site name and slogan, but if they are toggled off, we'll
  // just hide them visually.
  $variables['hide_site_name']   = theme_get_setting('features.name') ? FALSE : TRUE;
  $variables['hide_site_slogan'] = theme_get_setting('features.slogan') ? FALSE : TRUE;
  if ($variables['hide_site_name']) {
    // If toggle_name is FALSE, the site_name will be empty, so we rebuild it.
    $variables['site_name'] = check_plain($site_config->get('name'));
  }
  if ($variables['hide_site_slogan']) {
    // If toggle_site_slogan is FALSE, the site_slogan will be empty, so we rebuild it.
    $variables['site_slogan'] = filter_xss_admin($site_config->get('slogan'));
  }
  // Since the title and the shortcut link are both block level elements,
  // positioning them next to each other is much simpler with a wrapper div.
  if (!empty($variables['title_suffix']['add_or_remove_shortcut']) && $variables['title']) {
    // Add a wrapper div using the title_prefix and title_suffix render elements.
    $variables['title_prefix']['shortcut_wrapper'] = array(
      '#markup' => '<div class="shortcut-wrapper clearfix">',
      '#weight' => 100,
    );
    $variables['title_suffix']['shortcut_wrapper'] = array(
      '#markup' => '</div>',
      '#weight' => -99,
    );
    // Make sure the shortcut link is the first item in title_suffix.
    $variables['title_suffix']['add_or_remove_shortcut']['#weight'] = -100;
  }
}

/**
 * Implements hook_preprocess_HOOK() for maintenance-page.html.twig.
 */
function bartik_preprocess_maintenance_page(&$variables) {
  // By default, site_name is set to Drupal if no db connection is available
  // or during site installation. Setting site_name to an empty string makes
  // the site and update pages look cleaner.
  // @see template_preprocess_maintenance_page
  if (!$variables['db_is_active']) {
    $variables['site_name'] = '';
  }
  drupal_add_css(drupal_get_path('theme', 'bartik') . '/css/maintenance-page.css');
}

/**
 * Implements hook_process_HOOK() for maintenance-page.html.twig.
 */
function bartik_process_maintenance_page(&$variables) {
  $site_config = config('system.site');
  // Always print the site name and slogan, but if they are toggled off, we'll
  // just hide them visually.
  $variables['hide_site_name']   = theme_get_setting('features.name') ? FALSE : TRUE;
  $variables['hide_site_slogan'] = theme_get_setting('features.slogan') ? FALSE : TRUE;
  if ($variables['hide_site_name']) {
    // If toggle_name is FALSE, the site_name will be empty, so we rebuild it.
    $variables['site_name'] = check_plain($site_config->get('name'));
  }
  if ($variables['hide_site_slogan']) {
    // If toggle_site_slogan is FALSE, the site_slogan will be empty, so we rebuild it.
    $variables['site_slogan'] = filter_xss_admin($site_config->get('slogan'));
  }
}
...
?>

After:

<?php
...
/**
 * Implements hook_preprocess_HOOK() for page.html.twig.
 */
function bartik_preprocess_page(&$variables) {
  // Pass the main menu and secondary menu to the template as render arrays.
  if (!empty($variables['main_menu'])) {
    $variables['main_menu']['#attributes']['id'] = 'main-menu-links';
    $variables['main_menu']['#attributes']['class'] = array('links', 'clearfix');
  }
  if (!empty($variables['secondary_menu'])) {
    $variables['secondary_menu']['#attributes']['id'] = 'secondary-menu-links';
  }

  $site_config = config('system.site');
  // Always print the site name and slogan, but if they are toggled off, we'll
  // just hide them visually.
  $variables['hide_site_name']   = theme_get_setting('features.name') ? FALSE : TRUE;
  $variables['hide_site_slogan'] = theme_get_setting('features.slogan') ? FALSE : TRUE;
  if ($variables['hide_site_name']) {
    // If toggle_name is FALSE, the site_name will be empty, so we rebuild it.
    $variables['site_name'] = check_plain($site_config->get('name'));
  }
  if ($variables['hide_site_slogan']) {
    // If toggle_site_slogan is FALSE, the site_slogan will be empty, so we rebuild it.
    $variables['site_slogan'] = filter_xss_admin($site_config->get('slogan'));
  }
  // Since the title and the shortcut link are both block level elements,
  // positioning them next to each other is much simpler with a wrapper div.
  if (!empty($variables['title_suffix']['add_or_remove_shortcut']) && $variables['title']) {
    // Add a wrapper div using the title_prefix and title_suffix render elements.
    $variables['title_prefix']['shortcut_wrapper'] = array(
      '#markup' => '<div class="shortcut-wrapper clearfix">',
      '#weight' => 100,
    );
    $variables['title_suffix']['shortcut_wrapper'] = array(
      '#markup' => '</div>',
      '#weight' => -99,
    );
    // Make sure the shortcut link is the first item in title_suffix.
    $variables['title_suffix']['add_or_remove_shortcut']['#weight'] = -100;
  }
}

/**
 * Implements hook_preprocess_HOOK() for maintenance-page.html.twig.
 */
function bartik_preprocess_maintenance_page(&$variables) {
  // By default, site_name is set to Drupal if no db connection is available
  // or during site installation. Setting site_name to an empty string makes
  // the site and update pages look cleaner.
  // @see template_preprocess_maintenance_page
  if (!$variables['db_is_active']) {
    $variables['site_name'] = '';
  }
  drupal_add_css(drupal_get_path('theme', 'bartik') . '/css/maintenance-page.css');
  $variables['styles'] = drupal_get_css();

  $site_config = config('system.site');
  // Always print the site name and slogan, but if they are toggled off, we'll
  // just hide them visually.
  $variables['hide_site_name']   = theme_get_setting('features.name') ? FALSE : TRUE;
  $variables['hide_site_slogan'] = theme_get_setting('features.slogan') ? FALSE : TRUE;
  if ($variables['hide_site_name']) {
    // If toggle_name is FALSE, the site_name will be empty, so we rebuild it.
    $variables['site_name'] = check_plain($site_config->get('name'));
  }
  if ($variables['hide_site_slogan']) {
    // If toggle_site_slogan is FALSE, the site_slogan will be empty, so we rebuild it.
    $variables['site_slogan'] = filter_xss_admin($site_config->get('slogan'));
  }
}
...
?>

Affects: Module developers and themers

Thanks @ekl1773, that's very thorough :) This change notice should IMO be about color module, not Bartik. I think the key point for this change notice is this:

Color module doesn't require themes to call _color_html_alter and _color_page_alter anymore. These two functions have been removed, and themes that still use them will break.

IMO we can remove the first two before/after code snippets entirely - if someone is curious about the behind the scenes changes they can come to this issue and look at the patch. Otherwise this doesn't affect them, it still works the same as before with less code on their part.

And to trim up the second set of before/after code snippets, I would trim the code sample to only show the parts that hook up to color module - you could also consider renaming the bartik_ functions to MYTHEME_ or similar to make them generic. We should then end up with no code at all in the "after" snippet because no extra code is needed!

Created change record: https://drupal.org/node/2031831

Makes sense to me, thanks so much @ekl1773!