Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Issue #1898054 by pixelmord, jenlampton, steveoliver, EVIIILJ, c4rl, vlad.dancer, Fabianx, joelpittet, jwilson3, thedavidmeister, shanethehat, Cottser: Convert comment module to Twig.
Task
Use Twig instead of PHPTemplate
Remaining
- Patch needs review
- Profiling
Template path | Conversion status |
---|---|
core/modules/comment/templates/comment-wrapper.tpl.php | converted |
core/modules/comment/templates/comment.tpl.php | converted |
Testing steps
1. comment, comment_wrapper: Adding a single comment to a node and viewing it as attached to the node.
2. comment-block: Enable the block in a visible region, add comments as necessary to populate the block.
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#115 | 1898054-115.patch | 18.79 KB | star-szr |
#115 | interdiff.txt | 11.8 KB | star-szr |
#114 | interdiff.txt | 696 bytes | shanethehat |
#114 | twig-comment-1898054-114.patch | 30.59 KB | shanethehat |
#113 | twig-comment-1898054-113.patch | 30.64 KB | shanethehat |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedI'm looking at this as part of Drupalcon code sprint.
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedHere's a work-in-progress for getting the existing tpl.php files into html.twig format, and updating relevant "support" code like the preprocess + docs. Haven't done the theme functions yet.
Focussing on 1:1 match with current D8 html for now even though I saw a few things that could be improved in the template, I didn't want to be trying to fix two thing at once.
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedThis is a patch against core that replaces all tpl.php files and theme functions for the comment module with html.twig template files.
I removed the 'comment_preview' item from comment_theme() as I could not find a single place where it is used to theme anything - comment previews are actually rendered by comment.html.twig. Everything else has been converted as close to 1:1 resulting markup with current core as possible.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedI'll try to get this green. Apparently something's going wrong with previews in the test.
Comment #7
thedavidmeister CreditAttribution: thedavidmeister commentedProblem was bartik having a couple of comment templates that were using the old tpl.php style. Since those templates were identical to the core ones, I ripped them out and all my local tests started coming back green. Made a few other minor changes to fix a few exceptions and simplify the twig templates a bit too.
Comment #8
Fabianx CreditAttribution: Fabianx commentedWhy are we removing this?
This is not yet thoroughly discussed, but we are in the works of deprecating theme() (at least from preprocess).
Please use a render array like:
$variables['comments'] = '';
if ($items) {
$variables['comments'] = array('#theme' => 'item_list', '#items' => 'items');
}
That way it will be rendered just in the template and not yet in the preprocess.
The same could be done for the l() here, but due to performance I would leave it for now and do maybe as follow-up.
These are both no-ops (the ?:), because if it is FALSE the result is array() anyway.
Please use the longer form with:
isset(x)?x:array();
Thanks!
We might want to use either here:
{% if not comments
or
later
{% if comments is not empty
But it should be consistent ...
"@todo" is easier to find in the code by grepping for it.
Should be consistent with comment-block.html.twig.
Either use the long form here
{% if comments is not empty
or the short form above.
The
<div class="content"
is wrong and should include the content_attributes as else we might end up with two class="" statements.Might need to fix a markup test if you change it here.
This should use:
{% hide(...) %}
as
{{ is for printing.
Great start!
Please continue the nice work!
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commented- Removing 'comment_preview' because I really don't think it is doing anything. No existing theme function, no existing template file, no reference to it anywhere else in the code base.
- updated the item_list in comment_block to be render array style
- ignoring l() for now, happy to make a follow-up issue
- swapped out ?: with isset(), this is still necessary for now as Twig was throwing exceptions when I asked it to print NULL values. Happy to make a follow up issue to get Twig handling NULL better when printing, which will make preprocessors and templates both easier to write.
- using short form "if not comments" in comments-block.html.twig since I very much prefer the converse "if comments" over "if comments is not empty"
- The
<div class="content">
situation is due to copying core as of yesterday at midday, I've replaced with<div class="content {{ content_attributes.class }}"{{ content_attributes }}>
- using {% hide() %} in comment.html.twig
- using @todo instead of TODO
Patch attached. Testbot is currently taking close to 2 hours to complete requests but I'm getting 100% green running the comments tests on my local.
Comment #10
star-szrThanks for all your work on this, @thedavidmeister! I found some minor documentation fixes that can be done to move this further.
Let's go with "Preprocesses variables for…" for now for these, and they should end in a period per http://drupal.org/node/1354#drupal. We're working out the exact standards for documenting preprocess functions in #1913208: [policy] Standardize template preprocess function documentation.
Extra space after @see, please remove.
These
@see
references need to end in parens since they point to functions, i.e.@see template_preprocess()
, per http://drupal.org/node/1354#see. The@see
references were updated yesterday in the sandbox but need to be updated here as well to meet documentation standards.Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedComment #12
thedavidmeister CreditAttribution: thedavidmeister commentedStandards cleanup as requested by Cottser.
Comment #13
star-szrThanks @thedavidmeister! I should have mentioned this earlier but an interdiff would be very helpful as well :)
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedsorry, interdiff makes sense. I'll keep that in mind for the next revision/patch I do.
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commentedwhaaaat??
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commented#12: 1898054-twig-for-comments-12.patch queued for re-testing.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedI don't know what happened to the testbot. Fatal errors were coming from the contact form of all places and all I changed was a little whitespace and some comments... I re-tested it and everything came back green.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedi just got patchutils and ran the interdiff command... thanks for showing me that link Cottser.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedFollowup issue for converting l() to a render array #1914000: Convert l() call in template_preprocess_comment_block() into a render array to be handled by Twig..
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedI tried to replicate the problems I was having with exceptions for null values being printed into Twig templates. I think I must have been doing something wrong yesterday as I couldn't reproduce any problems at all tonight during tests.
It's a good thing, no need to create new follow-up issues and I simplified the preprocess functions a little bit in this patch. Nothing else has changed :)
Comment #22
star-szrContributors (via git blame of preprocess functions and template files as well as a quick issue search in the sandbox) added to issue summary.
@thedavidmeister - I noticed a couple other very minor tweaks as I read through the patch again. No need to set to "needs work" for these.
HTML should be all caps.
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentednp.
Comment #24
jenlamptonThis looks really close guys, but it looks like we deleted the PHPTemplate templates from the Bartik theme, but didn't add twig versions back in their place. Did they just get excluded from a patch by accident?
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commentedI removed them deliberately. Comment #7:
I couldn't see why we need templates in Bartik that are identical/duplicates of what's in core so I didn't add them back in. Is that really bad?
Comment #26
thedavidmeister CreditAttribution: thedavidmeister commentedPatch from #24.
Comment #27
steveoliver CreditAttribution: steveoliver commentedNice work. Here's some nitpicks.
This docblock language needs to be edited as per #1913208: [policy] Standardize template preprocess function documentation.
Same here.
Needs updated docblock as per new convention. I'd say maybe keep the @param since it's already there and still relevant.
We don't need to run these through the t filter. We aren't translating URLs. Just {{ user_login_url }} and {{ user_register_url }} inbetween raw
<a>
markup should do it.@file
Default theme implementation for a comments container.
render_var($content) comments are not relevant in Twig.
display_mode CONSTANTS may come through as int. Double check and update docs here accordingly.
{% if comments and node.type is not 'forum' %} ... get used to natural language in conditions. :)
As per Twig docs on filters, remove space surrounding pipe | symbol:
{{ 'Comments'|t }}, etc...
That's about all I see.
Comment #28
gillbates CreditAttribution: gillbates commented#4: 1898054-twig-for-comments-4.patch queued for re-testing.
Comment #29
gillbates CreditAttribution: gillbates commentedOops, didn't expect that to happen. I thought that link would run some sort of automated test. Don't know how to undo what I did. Please disregard.
Comment #30
jenlampton@thedavidmeister - the templates in core should not be the same as the ones in Bartik. If they look the same in what we've done - then we're doing something wrong. :)
We'll need to open the core .tpl.php files and the bartik .tpl.php files that are currently in 8, and do a diff. The same markup differences that exist in those files will need to exist in our twig versions.
For now, let's leave the templates in bartik alone (as .tpl.php files) until we figure out #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion
After that ges resolved we may need to replace those as part of this patch as well.
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commented#27 - thanks for that review. I'll look over it soon, just got back from a weekend away so I didn't see it earlier.
#30 - I did a diff, comment-wrapper.tpl.php is definitely identical in core and in bartik except the bartik one says "this is bartik's template". There are some differences in comment.tpl.php in bartik that I didn't originally see - I'll have to make sure they're reflected in the twig template. Having Bartik templates in place makes it impossible to see whether the core templates themselves are really passing tests or not, this is a serious mistake IMO, why on earth is the default theme used by core for tests overriding core templates and changing the markup?? in practice this allows tests to pass when they wouldn't for any theme that isn't overriding templates in the same way that Bartik does.
Comment #32
thedavidmeister CreditAttribution: thedavidmeister commentedI just opened #1939286: The theme that Drupal core runs tests in on d.o. (Bartik) should not be allowed to override core templates so we don't have to discuss whether Bartik "should" have it's own template files in this thread. I'll just try to convert everything 1:1 even though I hate that it's there at all :)
Comment #33
star-szr@thedavidmeister - I know #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig has been keeping you busy - mind if I work on a new patch for this issue?
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentedThat would be great actually, thanks for the offer! Hopefully I can get to some kind of closure on the l() issue soon and get back into making templates ;)
I was thinking that we need two patches for this thread, one with the Bartick templates in it that we intend to have committed and one without, so we can flag any failed tests against the core templates that won't be picked up by the bots with the Bartick templates in place but would still effect most end-users.
Comment #35
star-szrOkay, I'm on it. Expect a revised patch a bit later in the week.
Comment #36
star-szrFirst - a reroll, the patch from #26 no longer applied.
Comment #37
star-szrAddressed everything from #27 and did a bunch more documentation cleanup/updates (i.e. remove references to arrays from .html.twig files) and converted a couple theme() calls to render arrays.
This is the only thing mentioned in the review in #27 that was not changed, regarding comment-post-forbidden.html.twig:
I think these look just fine, I don't see how else to get the strings to t() for translation. Left as is, just removed the spaces surrounding the pipe character.
@todo:
Compare core vs. Bartik templates and make the Twig templates in this patch match those differences, would fix the below regression and probably others.
HEAD:
This patch:
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commented#37 - thanks for the update, much appreciated. If you're working on this again this weekend I'll try to find time to do a review for you once the Bartik templates are updated :)
Comment #39
star-szrActually, the "The constants below" and code sample should be part of the display_mode list item, otherwise this will make very little sense when looking at on api.d.o :)
Edit: maybe not… see http://api.drupal.org/api/drupal/core%21modules%21comment%21templates%21...
Comment #41
star-szrWell that's weird, it didn't like the render arrays I guess. Will check it out tonight.
Comment #42
andypostWill make a follow-up for #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #43
star-szrThis should fix all the test failures. Next up, Bartik!
'author' and 'parent_author' go through t(), so I think that's a no go to convert to render arrays for now. See #1706612: remove 'submitted' variable in templates for ease of theme development that would help with at least 'author'.
Also
{% node.type is not 'forum' %}
from #27 doesn't seem to be a thing in Twig,!=
works though.Comment #44
thedavidmeister CreditAttribution: thedavidmeister commented#43 - Yeah, apparently "node.type is not 'forum'" doesn't work because "is" in Twig is only related to Twig "tests" http://twig.sensiolabs.org/doc/tests/index.html and 'forum' is not a Twig test. I definitely remember going through all this in IRC with somebody - guess I forgot to bring it back here, sorry!
You could possibly do "node.type is not sameas('forum')" http://twig.sensiolabs.org/doc/tests/sameas.html but that's rather verbose, I think I like it less than !=
I also vaguely remember going through the render arrays in t() stuff back at Drupalcon when I first picked up this issue (t() hates render arrays)... I think there's a little double handling going on here, but at least we're ending up in the same place :)
Comment #45
star-szrYeah, I found sameas() as well but preferred !=. Thanks @thedavidmeister :)
Comment #46
star-szrUpdating status, need to update the Bartik templates.
Comment #46.0
star-szrUpdated issue summary.
Comment #47
star-szrTests seem to pass locally now without changing Bartik, let's see if testbot agrees.
Snuck in a couple comment.tpl.php -> comment.html.twig documentation changes as well.
Comment #47.0
star-szrAdd conversion summary table
Comment #47.1
star-szrUpdate remaining
Comment #48
star-szrThis could use manual testing to check our markup, if someone can write up brief testing steps and add to the summary (similar to #1898432: node.module - Convert PHPTemplate templates to Twig) that would be great.
Comment #49
thedavidmeister CreditAttribution: thedavidmeister commentedCottser left this assigned to himself but the status is "needs review". I'm assigning this to myself to review and test the most recent patch manually.
Comment #50
thedavidmeister CreditAttribution: thedavidmeister commentedNitpicks:
comment-wrapper.html.twig:
"The constants ...." is not supposed to be indented below "- node:" is it? It looks like it's supposed to be a new section - it was previously in comment-wrapper.tpl.php.
Is there a followup issue open for this on d.o.?
comment.html.twig:
Inside the preprocess this variable would be $comment->changed, not comment->changed.
* - picture: The comment author's picture.
user_picture?
* - classes: String of classes that can be used to style contextually through
attributes.class?
- There's no explanation of what the variables "parent" or "content_attributes" do in the documentation, they're just used in the template. comment.tpl.php had a nice discussion of the $parent_* variables.
comment-post-forbidden.html.twig
* Default theme implementation for a "you can't post comments" notice.
The preprocess was changed to "for comment forbidden notice templates." from "you can't post comments" notice. This should match that.
Technically it's only whether authenticated users can post comments. A site might allow some roles to post but not others. The user might be able to post once they have logged in but can_post_comments would still be false if they had some special role. Maybe this should say "Whether authenticated users can post comments once logged in."
This should be decided before we calling a template called "post-forbidden", not inside it - that or this template could be merged into comment-wrapper. This doesn't impact the conversion as it was always in the theme function but maybe we should open a followup ticket and add a @todo notice.
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commentedIf patches posted here could have a version posted with the Bartik templates deleted until #1595028: Convert tests using Standard profile to use Testing profile instead lands that would be great for speeding up reviews.
That is to say, for each re-roll upload one patch that doesn't touch Bartik that we intend to have committed, and one "test safe" patch with the Bartik templates deleted that we don't intend to commit so we can be *sure* that the testbot is not just happily passing everything by using the tpl.php files in the Bartik theme regardless of whether the Twig template is working or not. Both patches need to come back green before the "real" patch can be committed.
Also, I'd like to confirm before we move on - are we converting Bartik's comment templates in this issue or in #1938840: bartik.theme - Convert PHPTemplate templates to Twig? My vote is to handle that in the Bartik specific issue and really focus on core's handling of comments in this issue.
Comment #52
star-szrThanks for the thorough review @thedavidmeister. For testing the conversion in this issue, please use the Stark theme, and we'll handle the Bartik overrides in #1938840: bartik.theme - Convert PHPTemplate templates to Twig.
Comment #52.0
star-szrUpdate remaining and conversion table
Comment #53
star-szrI was originally hoping to update this patch, but currently have too many others on the go. If someone else wants to look at updating the patch based on the feedback in #50 that would be great :) Any questions just post here, even a partially updated patch would help move this forward though!
Comment #54
shanethehat CreditAttribution: shanethehat commentedWhen this makes it to commit, someone should look at decoupling comment-wrapper.html.twig from the forum module: #1285842: Forum module should implement its own node and comment templates to make theming easier
Comment #55
shanethehat CreditAttribution: shanethehat commentedComment #56
shanethehat CreditAttribution: shanethehat commentedComment #57
shanethehat CreditAttribution: shanethehat commentedThere are still a couple of calls to theme() in template_preprocess_comment()
$variables['author'] = theme('username', array('account' => $account));
$variables['parent_author'] = theme('username', array('account' => $account_parent));
Removing these causes array to string conversion exceptions in the tests. Should they be left as they are, or should the tests be altered to render the arrays before using them?
Comment #58
thedavidmeister CreditAttribution: thedavidmeister commented#57 - when you say "removing them" do you mean turning those variables into renderable arrays? Exactly which tests in what file are failing?
Comment #59
star-szrI think we need to keep those as theme() calls for now, they are passed around as strings.
Comment #60
shanethehat CreditAttribution: shanethehat commented#58 - When I run the comment tests with the those functions converted like so:
I get this back:
The vast majority of these exceptions are array to string conversions triggered by CommentTestBase::postComment calling drupal_post() with an array rather than a string for the author. This should be easy enough to fix for these tests, however...
#59 - I've not run the entire test suite with the changes above, so I have no idea how many things it would break. I think you're right and we should leave it for now, but this should maybe have a follow up issue?
Comment #61
shanethehat CreditAttribution: shanethehat commentedRemoves whitespace fails from #56
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commented#60 - indeed. Would you mind opening the followup issue and linking it from here?
Comment #63
shanethehat CreditAttribution: shanethehat commented#62 - Issue created #1972702: Remove calls to theme() from template_preprocess_comment()
Comment #64
star-szrLet's remove the curly braces here per http://drupal.org/node/1823416#variables-inline please, and use single quotes.
This is looking really good, thanks @shanethehat and @thedavidmeister for keeping this one moving :)
Comment #65
shanethehat CreditAttribution: shanethehat commentedComment #66
c4rl CreditAttribution: c4rl commentedI really don't think we need theme_comment_post_forbidden as a theme function at all. All its doing is returning a message with some links based on some conditions; there's no markup. I dug into the commit history of when this was committed back in 2002(!) and it really only serves the purpose of being a callback for messaging. See http://drupalcode.org/project/drupal.git/blob/92326261d16d7a4d02f97d70f9...
Few other nitpicks:
I think we should have an `else` statement here.
This causes unnecessary trailing whitespace because content_attributes.class is empty. We should probably solve this in preprocess. Ironically, the markup that the unpatched head creates is invalid anyway (double class attributes).
Comment #67
c4rl CreditAttribution: c4rl commentedI've made a few revisions here.
* I've removed any calls to theme() and replaced with render arrays
* I've deprecated theme_comment_post_forbidden and replaced with a callback that just returns the text of the message. As stated in #66, we really don't need this to go through the theme layer.
* Some other small clean-up mentioned in #66.
Comment #68.0
(not verified) CreditAttribution: commentedStart testing steps
Comment #69
c4rl CreditAttribution: c4rl commentedMight help if I checked my syntax. :)
The interdiff is with respect to #65
Comment #70
c4rl CreditAttribution: c4rl commentedComment #72
c4rl CreditAttribution: c4rl commentedI see what's going on. Render arrays aren't working as suspected. However, I don't think theme_username should apply to output in a t() function. Will revise and post again.
Comment #73
thedavidmeister CreditAttribution: thedavidmeister commented+1 for merging/deleting theme_comment_post_forbidden() but with the proposed comment_forbidden_message() in #69, what would the process of making modifications to this message be now that it's not in the theme system?
I can absolutely see a use-case for wanting to modify it. Once I had a client who wanted "post comments" changed to "leave a message".
Comment #74
c4rl CreditAttribution: c4rl commentedCalls to the forbidden message are in two places:
1. comment_node_view() which can be overridden via a subsequent custom hook_node_view()
2. comment_links(), which can be overridden via hook_comment_view().
I've also proposed #1975962: Move comment_links() into CommentRenderController
Comment #75
c4rl CreditAttribution: c4rl commentedThere are some things in here that I'd like to improve that aren't necessarily related to Twig, mainly the calls to theme('username'), that are used subsequently in t(). Nevertheless, these are relatively outside the scope of the conversion and for the sake of progress I'm leaving them there for now. We'll revisit later in a different issue.
Interdiff from #69.
Comment #76
thedavidmeister CreditAttribution: thedavidmeister commentedAre these hooks something we'd consider most "front end developers" capable/excited to be using to get consistent theming for comments?
My gut tells me we might be burying this message a little deep for the average site builder.
Comment #77
c4rl CreditAttribution: c4rl commentedThis is not an issue of "consistent theming for comments." This is an issue of the best way to override something. A theme function that is only delegating between the text that a link produces is overkill.
The "forbidden" message is but one link of several that is being populated by comment_node_view(), and is the only one that is subject to any sort of theming. That is, if you wish to change any of the "delete" "reply" "edit" or "X more comments" links, you have always had to alter these via a subsequent hook_node_view(). The fact that the "forbidden" link happens to be put through a theme function is inconsistent with the rest of the links (which are not put through theme functions).
Since the only thing that theme_comment_post_forbidden() is doing is some logic to determine the text that should populate the link and there is no larger markup purpose here, it is my recommendation that the theme function be eliminated. If someone really wishes to alter the text, they can use the alter hooks (or address it in the node_preprocessor), just as they would have to do to alter other links in any circumstance.
Comment #78
thedavidmeister CreditAttribution: thedavidmeister commented#77 - @c4rl, Unless I'm missing something, that link to your Twitter seems to be saying the same thing I was in #76?
Life is hard for a lot of themers I suppose. I was mostly concerned that these themers probably don't have a lot of experience with hook_entity_view() and related functions and so might not even think to look there for overriding what they might see as "regular theme markup".
Luckily this reminded me that while all these links are not individually rendered by theme functions, they do all end up running through theme_links() anyway (duh), so I applied the patch in #75 and did this:
Everything works as you'd expect, I can override forbidden comment links through the theme system with or without theme_comment_post_forbidden(), just not inside a template (which never existed in D7 anyway). That function really is 100% redundant with other things that already exist and do the job in a better/more generalised way.
I'm going to (continue to) +1 the recommendation to kill off this theme function, but without any reservations this time :)
Comment #79
c4rl CreditAttribution: c4rl commentedGlad you came around. :)
The point I make is that there are many ways to override data, not just the theming layer, and there are things that the theme layer should override and should not override. theme_comment_post_forbidden() was an abuse of the theme system, IMHO, because the prime use case isn't driven specifically to adjust the markup produced, but rather to change the text (just text!) of the link itself. I'm of the school of thought that it isn't necessarily the role of theme system to alter a text property of some business object but rather the job of an alter hook (and that core should represent best practices). If hook_entity_view() is too difficult to understand, let's make it easier to understand (in a separate issue) instead of providing perceived workarounds in the core theme layer (as this just makes Drupal less consistent and thus more difficult to learn).
Investigating the commit history, this theme callback evolved non-methodically over the years; not from a specific issue or use case. Many things in core have evolved over the years without asking "Why is this being done?" I tend to ask this question often. :)
Comment #80
thedavidmeister CreditAttribution: thedavidmeister commentedI was never really against removing it, or I would have set this to "needs work" :P I remember discussing with @jenlampton pulling it out right back when I was rolling the first half-patch in #3!
hook_entity_view() is not difficult to understand (if you're learning it in context of something you'd want to use it for) but it's certainly not most people's "go to" place for modifying markup either.
I'm personally fine with using alters where appropriate, but I really do like on a pragmatic level (whether it's intentional or pure coincidence) that themers can modify the whole comment system's front-end markup through the theme system if they really wanted to - for whatever reason that makes sense to them.
Comment #81
jenlamptonPatch review:
- I love the removal of the forbidden template
- the link in the comment-wrapper template should be changed to http://drupal.org/node/1783190
- comment markup is a mess (not our problem, see #1965650: Comment rendering broken: comment body rendered outside of the bubble and re-roll once that is resolved.
Blocked for now :(
Comment #82
star-szrI wondered about the way the comment looked but the markup matched up so I didn't question it. Thanks @jenlampton.
Comment #83
star-szrUpdated the link in the @todo in comment-wrapper.html.twig and slightly tweaked it.
Comment #83.0
star-szrDescribed testing steps, denoted removals
Comment #84
c4rl CreditAttribution: c4rl commented#81 @jenlampton
I tend to disagree with the premise of codeblocks, see my comment here #1783184-5: [meta] Use Twig template inheritance. Let's discuss in IRC and follow-up.
Comment #85
star-szrRemoved the todo (see #1757550-38: [Meta] Convert core theme functions to Twig templates for reasoning).
Second patch removes the change to Bartik's comment.tpl.php, it could possibly trip up the tests that use the standard profile though. If it passes, let's go with the "leave-bartik" one to avoid conflicts.
Comment #86
star-szrOkay, looking good. 1898054-85-leave-bartik.patch is the patch to review :)
Comment #87
thedavidmeister CreditAttribution: thedavidmeister commented@Cottser, you need to delete the Bartik templates completely in one of the patches you set for review if you want the comment tests to be run against the new, core Twig templates.
Comment #87.0
thedavidmeister CreditAttribution: thedavidmeister commentedRemove sandbox link
Comment #87.1
star-szrUpdating commit message
Comment #88
star-szrI suppose it's been a while, here goes.
The patch to review is still 1898054-85-leave-bartik.patch in #85 :)
Comment #89
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to .tpl.php templates rather than theme_ functions. See #1987396: Refactor & Clean-up comment.module theme functions for theme_ function conversion.
Comment #89.0
c4rl CreditAttribution: c4rl commentedDe-dupe @thedavidmeister :)
Comment #89.1
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #89.2
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #90
c4rl CreditAttribution: c4rl commentedComment #90.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #91
star-szrComment #92
star-szrHere are just the .tpl.php conversions.See below patch for the .tpl.php conversions.
Comment #93
star-szrOops, wrong patch. Ignore #92.
Comment #94
star-szrI'll be working on profiling this.
Comment #95
joelpittetNitpicks:
A few $'s left in the twig files, should be removed. Al la: http://drupal.org/node/1823416#variables
Please indent the contents of the if block.
Comment #96
idflood CreditAttribution: idflood commentedHere is a quick reroll with the corrections from #95.
Comment #98
idflood CreditAttribution: idflood commented#96: twig_comment-1898054-96.patch queued for re-testing.
Comment #99
star-szrThank you @idflood!
Changes look good, but we'll need to change e.g. comment->created to 'comment.created' - add single quotes, and period instead of arrow.
Comment #100
idflood CreditAttribution: idflood commentedHere is the patch with the correction described in #99.
Comment #101
joelpittetremoved the remaining $'s from the twig template, moved attributes to preprocess, removed some empty string settings from Cottser's diff. See how this plays...
Comment #103
idflood CreditAttribution: idflood commentedThe patch in #101 seems to be working well with the patch from #1938840: bartik.theme - Convert PHPTemplate templates to Twig.
Comment #104
joelpittetGood point, I did test that the day before and forgot... here's the same patch with bartik template applied. Thanks @idflood
Comment #105
joelpittetSame as #104 except only the comment files were pulled from #1938840-48: bartik.theme - Convert PHPTemplate templates to Twig
Comment #107
star-szrOkay, I was wrong about needing to initialize the variables. This rolls back those changes. Maybe we can gain back some performance via #1857946: Comment parent template variables are built twice.
Comment #108
star-szrSorry for the delay in posting this, here are the initial profiling results based on the patch in #93.
Tested with Stark theme, 50 comments on a node page - flat comments (no threading). APC class loader enabled.
The first result is HEAD (at the time of #93) vs. itself - to show the the baseline run used for comparing runs is accurate.
The second result is HEAD vs. #93.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518b13c95a609&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518b13c95a609&...
The changes since #93 should improve these numbers by at least a bit (somewhere in the neighbourhood of 2.8%) but I need to re-profile and also test threaded comments.
Comment #109
star-szrQuick reroll now that #1990732: template_preprocess_comment() is preparing the parent's author with the wrong comment's uid is in, going to look at re-profiling this.
Comment #110
shanethehat CreditAttribution: shanethehat commentedI'll run though the manual testing
Comment #111
shanethehat CreditAttribution: shanethehat commentedBut first another reroll
Comment #112
shanethehat CreditAttribution: shanethehat commentedManual testing for comment-wrapper and comment done. The only difference is the order of the attributes on the wrapper element, but all elements are present and correct.
Not testing comment block because it is a theme function so should be part of the acceptance critera for #1987396: Refactor & Clean-up comment.module theme functions
Before:
After:
Comment #113
shanethehat CreditAttribution: shanethehat commentedThis corrects the section tag attributes order.
<section id="comments" class="comment-wrapper">
I think this is good to go, after profiling.
Comment #113.0
shanethehat CreditAttribution: shanethehat commentedUpdated issue summary.
Comment #114
shanethehat CreditAttribution: shanethehat commentedAdding a change to the bartik theme that removes a trailing space from a class list. See #1938840: bartik.theme - Convert PHPTemplate templates to Twig #52-3
Comment #115
star-szrI think removing Bartik's templates from here should be fine now, we shouldn't be converting those in two places. Ran comment preview and user picture tests locally and both passed, so hopefully nothing else breaks. I should have probably removed them back in #107.
Unassigning because I'm not sure when I'll get back to profiling this.
Comment #116
ezeedub CreditAttribution: ezeedub commentedScenario #1: Using Stark, devel generate a dozen or so nodes and set the "Maximum number of comments per node" to 30. Then set site front page to node with most comments (also, edit node comment settings to "open"). Comparing 8.x (6718550c) to #115.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199dd4798f9f&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199dd4798f9f&...
Scenario #2: Using Stark, create a page view of the nodes created in scenario #1 (node with comments). Set site default front page. Again, comparing 8.x (6718550c) to #115.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...
Comment #117
thedavidmeister CreditAttribution: thedavidmeister commented2% slower? that's pretty bad. Does anyone have any idea why it would be so much slower because I don't see anything obvious in the patch..
Comment #118
Fabianx CreditAttribution: Fabianx commentedYes, I can explain:
It is the getContextReference that comes to haunt me now ...
Compare:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...
with
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...
These are 16 ms, which are splitted like:
- 7.5 ms getContextReference
- 5 ms getAttribute - of that are 2ms TwigReference::offsetGet
- around 3.5 ms internal overhead for twig_render_var
So I think we need to live with around 2%.
Comment #119
jenlamptonI'm turning this over for an Alex Pott review since it's been profiled and we just need a sign-off.
Comment #120
catch@ezeedub how many comments were on the node that you profiled? Looks like less than 30, whereas for example this issue has 120.
@Fabian am I reading your diff right that you profiled with 37 comments? That'd mean 0.5ms overhead per comment which seems a lot -i.e. 150ms for 300 comments. Also wondering whether any of this is specific to the comment conversion or whether we'll see the same overhead with other templates that are called a lot during the same page.
Reading through getContextReference() it's not doing much expensive it's just doing it 465 times...
Only potential micro-optimization I could see for now was:
What about just setting $this->twig_reference in __construct()? If it's not used then one call gets wasted during a request, but otherwise that's 400+ comparisons saved.
Comment #121
catchAlso on the patch itself:
Why the extra IDs/classes?
Forum module shouldn't really be referenced by the core template, also forum module allows any node type to be placed in forums so this'd be inconsistent if a site has a couple of different node types used. edit: I see this was already in the template and just moved, but we could just drop that here IMO.
Comment #122
thedavidmeister CreditAttribution: thedavidmeister commentedComment #123
thedavidmeister CreditAttribution: thedavidmeister commented#121 - @catch, checkout #1285842: Forum module should implement its own node and comment templates to make theming easier for the forum issue. Not sure that the conversion task should be taking that responsibility.
Comment #124
thedavidmeister CreditAttribution: thedavidmeister commented#120 -
I think the profiling here #1843746: Convert views/templates/views-view-field.tpl.php to Twig is relevant to that question.
Comment #125
thedavidmeister CreditAttribution: thedavidmeister commentedCould we actually profile 300 comments to see if it adds 150ms as per #120?
Comment #126
star-szrCan we please also test @catch's idea in #120 for a potential optimization here (and for other commonly used templates)?
Comment #127
jerdavisComment #128
star-szrFrom #121:
We are not adding anything here, it is just a straight conversion :)
The second point was addressed in #123, that is out of scope for this direct conversion.
Profiling is nice but actually micro-optimizing at this point wouldn't really be worth delaying this further IMO. Setting to RTBC.
Edit: I did roll the last patch but it was just removing code and @Fabianx gave this a +1 :)
Comment #129
cweagans+1 from me as well.
Comment #130
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
We have a follow-up to address performance #2002094: Improve performance of comment.html.twig
Comment #131
alexpottCommitted 5f9bee7 and pushed to 8.x. Thanks!
Comment #132.0
(not verified) CreditAttribution: commentedRemoved manual testing, added profiling