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_comment
Twig Templates Modified
comment.html.twig
Comment | File | Size | Author |
---|---|---|---|
#37 | move_comment_classes-2329783-36.patch | 3.65 KB | lauriii |
#37 | interdiff-2329783-23-36.txt | 1.54 KB | lauriii |
Comments
Comment #1
davidhernandezComment #2
lauriiiComment #3
lauriiiComment #4
lauriiiAdded
vocabulary_name
to twig commentsComment #5
star-szr[redacted and moved to the proper issue]
Comment #6
star-szrOops, #4 belongs to #2329775: Move taxonomy term classes from preprocess to templates, disregard my comment.
Comment #7
lauriiiUps, posted my patch to wrong issue :D Ignore patch in comment #4
Comment #8
lauriiiWhere did that file come from?
Comment #9
RainbowArrayIs there a way to hide or delete the patches in #4 and #7? It's very confusing.
Comment #10
star-szrRe-adding #3 to the issue summary. d.o ghosts--
Comment #11
star-szrThis is looking really good, just a couple red spots jumped out at me:
Trailing whitespace, noooo!
Comment #12
lauriiiThere, I Fixed It!
Comment #13
star-szrI can't find any fault here, looks good. Thanks @lauriii!
Comment #16
lauriiiComment #17
star-szrSorry I missed this before. Checking for any comment author ID is not the same as checking that this ID is the same as the node author ID. So that needs updating.
Comment #18
lauriiiAs I discussed on IRC with @Cottser I fixed and moved functionality he mentioned on his comment #17.
Comment #20
lauriiiFixed tests
Comment #23
lauriiiReroll
Comment #24
guciek27 CreditAttribution: guciek27 commentedThe patch looks good, but there is one more class that is used.
file: comment.module
line: 765
Comment #25
lauriiiLink is generated in the preprocess so we cannot move that to template. Putting back to needs review.
Comment #26
lauriiiComment #27
BarisW CreditAttribution: BarisW commentedCode looks good and applies nicely.
To test it, I've copied the HTML of a comment before and after the patch and ran a diff. No changes in the HTML :)
+1 from me.
Comment #28
rteijeiro CreditAttribution: rteijeiro commented+1 RTBC
Comment #29
LewisNyman CreditAttribution: LewisNyman commented+1 RTBC
Tested in Stark, screenshot attached.
Comment #32
lauriiiPutting back to RTBC
Comment #33
webchickSo just a small process check question... should these base classes with all the classes defined not be going into themes/classy these days, and the module's template file basically class-less?
Comment #34
lauriiiThis issue is only for moving classes to templates. There is phase 2 where we will move templates with classes to classy and remove classes from module templates.
Comment #35
star-szrHere is the phase 2 issue.
Comment #36
alexpottDoesn't this mean that the permalink class is no longer on the title but on the entire article? Yep manual testing reveals unexpected differences in the html.
Comment #37
lauriiiGood catch!
Comment #38
LewisNyman CreditAttribution: LewisNyman commentedI diffed the output in Bartik as Alex did and found no different apart from the position of the classes, we also removed a duplicate clearfix class. That's good.
Comment #39
alexpottI think this will break for comments on a user since a user does not implement EntityOwnerInterface. I think we need to add something like
commented_entity.getOwnerId is defined
in the condition. As far as I can see we have no tests for adding a comment field to an entity type that does not implement EntityOwnerInterface. We should get a followup to do just that.Comment #40
jamesquinton CreditAttribution: jamesquinton commentedAdded commented_entity.getOwnerId is defined.
Comment #41
lauriiiGood work on the patch! #40 fixes #39. More info about is defined in the Twig Documentation.
Comment #42
joelpittet@alexpott that is defined is quite useless it seems with strict variables turned off like we have.
Here's a quick test you can try:
Output is
false : true
This is one of the reasons we chose to set strict_variables to FALSE in core.
If you turn strict_variables on... then you get:
Fatal error: Uncaught exception 'Twig_Error_Runtime' with message 'Method "missing" for object "HasNoMethod" does not exist in ...
Comment #43
davidhernandezSo...should we stick to the patch in #37 then?
Comment #44
joelpittetYes, I think we should stick with #37
Comment #46
lauriiiComment #47
rteijeiro CreditAttribution: rteijeiro commentedSo as it seems #37 patch (#2329783-37: Move comment classes from preprocess to templates) is green, let's go back to RTBC.
Comment #48
alexpottThis issue is a unfrozen change (markup) as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed e3e7044 and pushed to 8.0.x. Thanks!
Comment #51
lauriiiPutting back to fixed.
Comment #52
Wim LeersThis perpetuates the "user ID zero is the anonymous user" check which IMHO makes no sense.
This should've used
$comment->getOwner->isAnonymous()
.Comment #53
Fabianx CreditAttribution: Fabianx commentedLets file a follow-up for that, because this issue was a straight port from the preprocess code.
Comment #54
joelpittet@Wim Leers and @Fabianx created follow-up feel free to update the IS:
#2372909: Comments to check '$comment->getOwner->isAnonymous()' instead of assuming anonymous is ID 0