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_admin_block_content
template_preprocess_status_report
template_preprocess_system_themes_page

Twig Templates Modified

admin-block-content.html.twig
seven/templates/admin-block-content.html.twig
status-report.html.twig
system-themes-page.html.twig

Files: 
CommentFileSizeAuthor
#18 2329851-18.patch8.94 KBJolidog
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2329851-18.patch. Unable to apply patch. See the log in the details link for more information. View
#16 2329851-16.patch8.94 KBJolidog
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,763 pass(es). View
#13 2329851-13.patch8.94 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,706 pass(es). View
#13 interdiff-2329851-10-13.txt662 byteslauriii

Comments

davidhernandez’s picture

Issue tags: +FUDK
lanchez’s picture

Assigned: Unassigned » lanchez
lanchez’s picture

Assigned: lanchez » Unassigned
Status: Active » Needs review
FileSize
9.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,831 pass(es). View

This patch is pretty much doing what was needed. Some classes were left behind in template_preprocess_system_themes_page in screenshots and links theme arrays. I don't really know what to do with that kind of usage situation.

Cottser’s picture

Status: Needs review » Needs work

Thanks @lanchez, this was a tricky one. Here's an initial review. My overall feeling is I think we may want to scale back a bit (mostly thinking of the status report) but if other folks are on board that's fine with me.

  1. +++ b/core/modules/system/system.admin.inc
    @@ -53,11 +53,10 @@ function _system_is_incompatible(&$incompatible, $files, Extension $file) {
         $compact = system_admin_compact_mode();
    ...
    +    $variables['compact'] = ($compact) ? TRUE : NULL;
    

    Why not $variables['compact'] = $compact; here? system_admin_compact_mode() returns a boolean.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -374,7 +373,7 @@ function template_preprocess_system_themes_page(&$variables) {
    -    $theme_group['attributes'] = new Attribute(array('class' => array('system-themes-list', 'system-themes-list-' . $state, 'clearfix')));
    
    +++ b/core/modules/system/templates/system-themes-page.html.twig
    @@ -28,10 +30,25 @@
    +        'system-themes-list-' ~ theme_group.state|clean_class,
    

    $state didn't go through a CSS-ifier before, not sure it should after.

  3. +++ b/core/modules/system/templates/admin-block-content.html.twig
    @@ -9,14 +9,21 @@
    + * - compact: This is true, if compact mode is turned on.
    
    +++ b/core/themes/seven/templates/admin-block-content.html.twig
    @@ -11,13 +11,20 @@
    + * - compact: This is true, if compact mode is turned on.
    

    I think we can lose the comma at least.

  4. +++ b/core/modules/system/templates/status-report.html.twig
    @@ -10,13 +10,26 @@
    - *   - severity_class: The HTML class of the severity.
    + *   - severity_status: Indicates the severity status. This can be:
    + *     * info
    + *     * ok
    + *     * warning
    + *     * error
    

    Maybe "This can be one of:" and the list items should use hyphens instead of asterisks per https://www.drupal.org/node/1354#lists.

  5. +++ b/core/modules/system/templates/status-report.html.twig
    @@ -10,13 +10,26 @@
    +{# severity_status : class #}
    +{%
    +  set severities = {
    +    'info' : 'info',
    +    'ok' : 'ok',
    +    'warning' : 'warning',
    +    'error' : 'error',
    +  }
    +%}
    

    I think the code comment here confused me, if we want to document this I think it should be a complete sentence explaining the array. Should be no space between the array key and the colon per Twig coding standards: http://twig.sensiolabs.org/doc/coding_standards.html

  6. +++ b/core/modules/system/templates/system-themes-page.html.twig
    @@ -28,10 +30,25 @@
    +    <div {{ theme_group.attributes.addClass(theme_group_classes) }}>
    

    Extra space between <div and {{ here.

lanchez’s picture

Assigned: Unassigned » lanchez

Thanks for review!

I will make another patch, but one comment on system_admin_compact_mode(). It does not work right right now, since it really returns the value of the cookie (string). So no boolean there and thats why the workaround is in place. The D7 version of the function works fine.

lanchez’s picture

Assigned: lanchez » Unassigned
Status: Needs work » Needs review
FileSize
8.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es). View
3.29 KB

I did a few modifications:
1. Didn't yet change that yet -> could someone confirm if system_admin_compact_mode() works as intended or not?
2. Removed clean_class
3. Fixed comment to: - compact: True if compact mode is turned on.
4/5. Removed whole array. I don't even remember why I did it that way. Now it just prints severity_status|clean_class
6. Removed space.

Cottser’s picture

Status: Needs review » Needs work

@lanchez, thanks. That's looking much better to me! For #1 adding a code comment to explain would be good for the scope of this issue and we can check for an existing issue or file a new one for the incorrect behaviour/docs.

  1. +++ b/core/modules/system/templates/admin-block-content.html.twig
    @@ -9,7 +9,7 @@
    - * - compact: This is true, if compact mode is turned on.
    + * - compact: True if compact mode is turned on.
    
    +++ b/core/themes/seven/templates/admin-block-content.html.twig
    @@ -11,7 +11,7 @@
    - * - compact: This is true, if compact mode is turned on.
    + * - compact: This is true if compact mode is turned on.
    

    I really like the the "True if…" one better! Can we make them both consistent please?

  2. +++ b/core/modules/system/templates/status-report.html.twig
    @@ -38,7 +25,7 @@
    -    <tr class="{{ severities[requirement.severity_status] }}">
    +    <tr class="{{ severity_status|clean_class }}">
    

    Doesn't this need to be requirement.severity_status? Again, not sure if we need clean_class in this case.

lanchez’s picture

Status: Needs work » Needs review
FileSize
8.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View
1.75 KB

Doh, stupid mistakes from me :)

I fixed the comment and severity_status class (and removed the clean_class, since its a manual string in code). Also added the comment you suggested.

lauriii’s picture

Nice work on the patch!

+++ b/core/modules/system/system.admin.inc
@@ -53,11 +53,12 @@ function _system_is_incompatible(&$incompatible, $files, Extension $file) {
+    // Check if compact mode is turned on.
+    // NOTICE: system_admin_compact_mode() seems to return a string
+    // and not a boolean.
     $compact = system_admin_compact_mode();
-    $variables['attributes'] = array('class' => array('admin-list'));
-    if ($compact) {
-      $variables['attributes']['class'][] = 'compact';
-    }
+    $variables['compact'] = ($compact) ? TRUE : NULL;

What are we trying to tackle exactly with this change? I think we instead need to fix the function or documentation for the function.

lauriii’s picture

FileSize
8.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,695 pass(es), 0 fail(s), and 207 exception(s). View
2.33 KB
mikl’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed, nice work :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2329851-10.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
662 bytes
8.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,706 pass(es). View
sqndr’s picture

+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -15,6 +15,10 @@
+ *     - is_default: Boolean indicating whether the theme is default theme or
...
+ *     - is_admin: Boolean indicating whether the theme is the admin theme or
+ *       not.

Change to: is_default: Boolean indicating whether the theme is the default theme not.

+++ b/core/modules/system/templates/admin-block-content.html.twig
@@ -9,14 +9,21 @@
+ * - compact: Boolean indicating whether compact mode is turned on or not.

+++ b/core/themes/seven/templates/admin-block-content.html.twig
@@ -11,13 +11,20 @@
+ * - compact: True if compact mode is turned on.

I would change the second documentation part to be the same as the first one. That would make sense, right? True if compact mode is turned on. would then just become Boolean indicating whether compact mode is turned on or not., like the first message.

sqndr’s picture

Status: Needs review » Needs work
Issue tags: -frontendunited, -FUDK +Novice

Tagging this as needs works again, removing some of the tags and adding the Novice tag, since fixing the remaining things would be a novice task.

Jolidog’s picture

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

Patch Reroll and comments fixed.

sqndr’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/system-themes-page.html.twig
@@ -15,6 +15,10 @@
+ *     - is_default: Boolean indicating whether the theme is the default theme or
+ *       not.

Thanks Jolidog for fixing the documentation. I've got to nitpick a little more… The or should be on the next line, according to the Drupal coding standards; since it's exceeding the 80 characters per line limit.

Jolidog’s picture

Status: Needs work » Needs review
FileSize
8.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2329851-18.patch. Unable to apply patch. See the log in the details link for more information. View

Thanks sqndr for spotting this, "or" is now on the next line.

mbroere’s picture

Doing manual review whit Kaleidoscope.

lauriii’s picture

Issue tags: +sprint
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I manually reviewed the patch to verify the classes still appear where they should and nothing is broken. BANANA!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 79f952a and pushed to 8.0.x. Thanks!

  • alexpott committed 79f952a on 8.0.x
    Issue #2329851 by lanchez, lauriii, Jolidog | davidhernandez: Move...

Status: Fixed » Needs work

The last submitted patch, 18: 2329851-18.patch, failed testing.

lauriii’s picture

Status: Needs work » Fixed

Putting back to fixed state

Status: Fixed » Closed (fixed)

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