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_image
template_preprocess_image_style

Twig Templates Modified

image.html.twig
image-style.html.twig

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

Issue tags: +FUDK
pakmanlh’s picture

Assigned: Unassigned » pakmanlh

Workin on it

pakmanlh’s picture

Assigned: pakmanlh » Unassigned
Status: Active » Needs review
FileSize
1.68 KB

I made a first approach, I don't know if is a correct way, but I thought it was suitable rewrite template_preprocess_image_style function to make it work like template_preprocess_image and it use the attributes variable instead of image variable.
Also I don't see anything to change on template_preprocess_image function.

Status: Needs review » Needs work

The last submitted patch, 3: move_image_classes_to_template-2329771-3.patch, failed testing.

star-szr’s picture

I don't think that's going to work, the output is no longer going through #theme image. Scale back the changes to only moving classes from preprocess to templates please :)

Edit: yeah, crossposted with testbot.

lauriii’s picture

Moving only the class is bit of a problem because image is being rendered before passing it to template

davidhernandez’s picture

Yeah, template_preprocess_image is in theme.inc, but image module creates template_preprocess_image_style, which adds the classes and other attributes. It prints them in image.html.twig. image-style.html.twig doesn't seem to do anything. We would need the 'style_name' variable to get to the image.html.twig template.

davidhernandez’s picture

The style_name variable is at the image-style template. The image attribute is there as an array, not an attribute object, so I don't think it makes sense to try to add it there?

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Ok, I did this by adding another variable to image. Not sure if it is the best approach. I did test it with a custom image style, and it seems fine.

Status: Needs review » Needs work

The last submitted patch, 9: move_image_classes_from-2329771-9.patch, failed testing.

davidhernandez’s picture

The failures just look to be because the attribute order changed. I'll try to fix it, but I'm having trouble getting my local testing working right now...

star-szr’s picture

Makes sense!

+++ b/core/modules/system/templates/image.html.twig
@@ -5,10 +5,16 @@
+ * - style_name: The name of the image style applied.

This should be marked as optional, because it won't always be there.

davidhernandez’s picture

Is there a particular convention for listing something as optional in a comment?

star-szr’s picture

- style_name: (optional) The name of the image style applied.

Ref. https://www.drupal.org/node/1354#lists

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
7.24 KB

Fixed the comment and the broken tests. (Order of the attributes changed.)

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll, looks like a patch context conflict with #2333395: Add sizes to template_preprocess_image.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
7.26 KB

reroll

davidhernandez’s picture

Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 17: move_image_classes_from-2329771-17.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
7.28 KB

Oops, when I merged I chopped out the variable added in the change from the other issue.

Status: Needs review » Needs work

The last submitted patch, 20: move_image_classes_from-2329771-20.patch, failed testing.

lauriii’s picture

star-szr’s picture

+++ b/core/modules/image/image.module
@@ -281,15 +281,13 @@ function template_preprocess_image_style(&$variables) {
+    '#style_name' => drupal_html_class($variables['style_name']),

For actually making use of this variable in the template, it's probably going to make more sense to not run this through drupal_html_class beforehand so that it matches up with the image style machine name. Can we just use |clean_class in the template instead when building the class?

davidhernandez’s picture

Yeah I thought about using clean_class instead but then must have gotten lazy. I agree with that change.

lauriii’s picture

FileSize
913 bytes
7.31 KB

Now we're using clean_class :)

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and manual testing checks out (and it has quite a bit of test coverage as well as we can see). RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2bccde8 and pushed to 8.0.x. Thanks!

  • alexpott committed 2bccde8 on 8.0.x
    Issue #2329771 by davidhernandez, lauriii, pakmanlh: Move image classes...

Status: Fixed » Closed (fixed)

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