Move classes out of the preprocess functions and into the Twig templates. Use the addClass() attribute method to add classes in the template. Use the clean_class filter to filter class names, if necessary. Maintain all existing functionality and ensure all existing class names are still in the markup, even ones that are inherited.

See the following issues for more detailed examples:
#2217731: Move field classes out of preprocess and into templates
#2254153: Move node classes out of preprocess and into templates

See this change record for information about using the addClass() method:
https://www.drupal.org/node/2315471

See this change record for more information about the phase 1 process of moving class from preprocess to templates:
https://www.drupal.org/node/2325067

Preprocess Functions Modified

template_preprocess_links

Twig Templates Modified

links.html.twig

Comments

davidhernandez’s picture

Issue tags: +FUDK
Sutharsan’s picture

Assigned: Unassigned » Sutharsan
Sutharsan’s picture

Assigned: Sutharsan » Unassigned
Status: Active » Needs review
FileSize
2.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,608 pass(es), 1 fail(s), and 0 exception(s). View

Link keys used as classes moved to twig template.
I did not touch the header class code (below) in template_preprocess_links , as I think it will be removed in #2285493: Remove deprecated 'class' property from #theme 'links' and #theme 'menu_tree' heading arrays.

      // @todo Remove backwards compatibility for $heading['class'].
      if (isset($heading['class'])) {
        $heading['attributes']['class'] = $heading['class'];
      }
mdrummond’s picture

+++ b/core/includes/theme.inc
@@ -1047,6 +1043,7 @@ function template_preprocess_links(&$variables) {
+      $li_attributes = array();

I'm not clear what this is doing?

Status: Needs review » Needs work

The last submitted patch, 3: drupal-links-classes-2329763-3.patch, failed testing.

Sutharsan’s picture

It just moves $li_attributes = array(); approx. 15 lines down. This is the resulting code:

   foreach ($links as $key => $link) {
      $item = array('key' => $key);
      $link += array(
        'href' => NULL,
        'route_name' => NULL,
        'route_parameters' => NULL,
        'ajax' => NULL,
      );

      $keys = array('title', 'href', 'route_name', 'route_parameters');
      $link_element = array(
        '#type' => 'link',
        '#title' => $link['title'],
        '#options' => array_diff_key($link, array_combine($keys, $keys)),
        '#href' => $link['href'],
        '#route_name' => $link['route_name'],
        '#route_parameters' => $link['route_parameters'],
        '#ajax' => $link['ajax'],
      );

      // Handle links and ensure that the active class is added on the LIs, but
      // only if the 'set_active_class' option is not empty.
      $li_attributes = array();
      if (isset($link['href']) || isset($link['route_name'])) {
        if (!empty($variables['set_active_class'])) {

          // Also enable set_active_class for the contained link.
          $link_element['#options']['set_active_class'] = TRUE;

          if (!empty($link['language'])) {
            $li_attributes['hreflang'] = $link['language']->id;
          }

[edit: move verbose explanation]

Sutharsan’s picture

Status: Needs work » Needs review

Back to needs review.

Cottser’s picture

FileSize
5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2329763-8.patch. Unable to apply patch. See the log in the details link for more information. View
2.9 KB

Made the necessary test updates, this should be green now.

Cottser’s picture

This is looking good to me now, needs more reviews though.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Tested this manually and classes seems to be the same after applying the patch!

catch queued 8: 2329763-8.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2329763-8.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,880 pass(es). View

Reroll

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

I checked manually and everything comes out the same. I don't see any other issues.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2329763-13.patch, failed testing.

Status: Needs work » Needs review

lauriii queued 13: 2329763-13.patch for re-testing.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

putting back

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.inc
@@ -975,7 +975,7 @@ function template_preprocess_links(&$variables) {
+      $item = array('key' => $key);

+++ b/core/modules/system/templates/links.html.twig
@@ -15,6 +15,7 @@
+ *   - key: The link key. To be used as link class.

@@ -45,7 +46,7 @@
+      <li{{ item.attributes.addClass(item.key|clean_class) }}>

Key seems to be a really nondescriptive name. Let's call it css class since the docs of template_preprocess_links() says The key for each link is used as its CSS class

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,438 pass(es). View

Rerolled and fixed #18

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Tested manually, and seem to still be getting the same classes. Code looks good. #19 includes the key name change.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.inc
@@ -978,16 +978,13 @@ function template_preprocess_links(&$variables) {
+      $item = array('css_class' => $key);

+++ b/core/modules/system/templates/links.html.twig
@@ -15,6 +15,7 @@
+ *   - css_class: The link CSS class.

@@ -45,7 +46,7 @@
+      <li{{ item.attributes.addClass(item.css_class|clean_class) }}>

I think we should change the variable name to link key to say what it is rather than what it is used for. And that way doing the clean_class in twig and not using drupal_html_class in the preprocess function makes sense.

davidhernandez’s picture

So change it from css_class to link_key?

Jens-0’s picture

FileSize
4.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,533 pass(es), 21 fail(s), and 4 exception(s). View

Changed variable name as suggested from css_class to link_key.

lauriii’s picture

Status: Needs work » Needs review

Sending to testbot :)

Status: Needs review » Needs work

The last submitted patch, 23: move_links_classes_from-2329763-23.patch, failed testing.

Jens-0’s picture

FileSize
4.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,490 pass(es). View
Jens-0’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Fixes #21

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.inc
@@ -978,16 +978,13 @@ function template_preprocess_links(&$variables) {
-      $item = array();
+      $item = array('link_key' => $key);

I think I've missed a trick here. There is no need for this extra variable. We just shouldn't remove the keys from $variables['links']. So

      // Add the item to the list of links.
      $variables['links'][] = $item;

becomes

      // Add the item to the list of links.
      $variables['links'][$key] = $item;

Then we can use the key in the twig template.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
4.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,496 pass(es). View

Maybe we finally could get this in ;)

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

#30 has the change in #29

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 45ebdf0 on 8.0.x
    Issue #2329763 by lauriii, Jens-0, Cottser, Sutharsan: Move links...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.