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 #1898466 by joelpittet, webthingee, W1n5t0n45, ry5n, ezeedub, lokeoke, Cottser | c4rl: Update.module - Convert theme_ functions to Twig.
Task
Use twig templates instead of theme_ functions for the update module.
To test this code
- Visit /admin/reports/updates or Reports > Status report and make sure that your report renders correctly
Theme function name/template path | Conversion status |
---|---|
theme_update_last_check | converted |
theme_update_manager_update_form | Removed, needed markup moved into form callback. |
theme_update_report | converted, troubleshooting |
theme_update_status_label | converted |
theme_update_version | Moved to it's own issue: #2264833: Convert theme_update_version to update-version.html.twig |
Related
#1757550: [Meta] Convert core theme functions to Twig templates
#1938934: Convert update theme tables to table #type
#2298039: Remove all PNG fallbacks for SVG
Followup: #2099293: Replace table with available module updates with better markup
Followup: #2287031: Change lcase translatable strings to ucfirst
Comment | File | Size | Author |
---|---|---|---|
#151 | update_module_convert-1898466-151.patch | 27.94 KB | steveoliver |
#151 | update_module_convert-1898466-148-151--interdiff-do-not-test.patch | 3.54 KB | steveoliver |
#149 | update-report.png | 247.21 KB | joelpittet |
#149 | updates-after-norm.txt | 24.51 KB | joelpittet |
#149 | updates-before-norm.txt | 24.27 KB | joelpittet |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
designtodigital CreditAttribution: designtodigital commentedAssigning this to me.
Comment #3
myers_d CreditAttribution: myers_d commentedReassigning this issue to my personal account.
Comment #4
tlattimore CreditAttribution: tlattimore commentedDoes look like this has been worked on in a month. I'll take a crack at it.
Comment #5
myers_d CreditAttribution: myers_d commentedMy apologies for not getting to this sooner... Just switched jobs and my free time is much less. tlattimore... thanks for picking up my slack.
Comment #6
carsonblack CreditAttribution: carsonblack commentedComment #7
W1n5t0n45 CreditAttribution: W1n5t0n45 commentedI made a patch for this, I believe it works but I am having trouble testing it.
Comment #9
carsonblack CreditAttribution: carsonblack commentedI have theme_update_last_check() and theme_update_version() converted.
Comment #10
tlattimore CreditAttribution: tlattimore commented@carsonblack - any progress on this? I'd be glad to help out on itif you'd like.
Comment #11
carsonblack CreditAttribution: carsonblack commentedA little bit, still on theme_update_report(). Should have this wrapped up by Sunday, 3/24/2013
Comment #12
star-szrAny progress you can share @carsonblack? Even posting a work-in-progress patch would be a great step forward for this issue. If not, we can have someone else (maybe @tlattimore?) take a crack at it.
Comment #13
joelpittetupdate_last_check i
s very similar to localelocale_translation_last_check
Save an extra
<p>
tag... maybe these two theme functions could be consolidated?This patch does that one conversion anyways.
In a bit of a rush so this doesn't even touch
theme_update_report()
But this patch should apply for anybody who want's to take this further. Also running into some errors with "Undefined index: reason" which I am sure will show up in the testbot
Comment #15
joelpittetSo this will probably pass, strange it needed the file in the update_theme definition but didn't for the theme_ functions...
Made a bit of a start on update_report, but still not converted... so if this passes, turn it back to needs work.
Comment #16
star-szrThanks @joelpittet, you rock. Back to CNW as requested in #15.
Comment #17
joelpittetForgot to mention update_version was removed from this conversion due to #type=>table duplicate stuff.
#1938934: Convert update theme tables to table #type
Comment #18
joelpittet@ry5n is going t add update_version to the twig again... discussed a bit with @Cottser on IRC and for now a straight twig conversion of this would be fine... possible down the road of making it a private function that calls #theme=>table__update_version would be more ideal but it's not a form table so doesn't need type=>table conversion. Closed that issue as won't fix for now. Hope we made the right move there.
Comment #19
joelpittettheme_update_manager_update_form
was removed because it was not necessary and moved the code to the form callback.Comment #20
ry5n CreditAttribution: ry5n commentedWorked on this with @joelpittet today (thanks Joel!). Got much of the conversion done for theme_update_report, but it’s not working yet. I won’t have time to come back to this for a while, so am posting progress here.
Comment #20.0
ry5n CreditAttribution: ry5n commentedadded type table for update_version
Comment #21
joelpittetPicking this up because I worked through much of it with @ry5n. update_reports was a beast to get through and this is a great start. Thanks for helping start that conversion!
Comment #22
ry5n CreditAttribution: ry5n commentedThanks Joel! Hopefully it’s on the right track.
Comment #23
star-szrTagging.
Comment #24
joelpittetSo there is an extra class attribute in the template_preprocess_update_version() which would be fixed by
http://drupal.org/node/1938430#comment-7240452
#1938430: Don't add a default theme hook class in template_preprocess() which adds the module name as a class.
Here's just a patch of the
update_version()
bit that was tweaked from #20, to see if it will pass. Still working on theupdate_report()
Comment #25
joelpittetComment #26
joelpittetOk the beast is in:
update_report()
. This may not pass but it's pretty close, if it doesn't I'll pick it up over the weekend.Comment #28
joelpittetSo having trouble with context within loops again...
#1867090: Nested Twig 'for' loops behaving strange
I left one trick in this patch to "fix" a few errors but this really needs to be looked into.
Comment #30
carsonblack CreditAttribution: carsonblack commentedoof , sorry Cottser, and others... Just got slammed with a bunch of other work. I'll check out the patches and see what I can do!
Comment #31
joelpittet@carsonblack no worries, thanks for getting it started. Could you look at that exception? Most of the fails are related to #1867090: Nested Twig 'for' loops behaving strange which I am working on tracking down right now... it looks like it's a drupal issue not a twig issue.
Comment #32
joelpittetSlightly better but still a couple of issues. I had to put a theme() call back in and write all the nested loop 'set' hacks to get things working.
Comment #33.0
(not verified) CreditAttribution: commentedAdd conversion summary table
Comment #34
star-szrDown to 3 fails is quite an achievement I'd say, fantastic work on this @joelpittet :)
Hate to bring this up now, but…
If at all possible we should be using t and join filters in the template for this.
Comment #35
joelpittet@Cottser, I think the answer is yes, if you look at previous patches it was in there, the reason I kept that in preprocess was because t('Includes: %includes') and t('Includes:') were different and wasn't sure I should change them both to {{ 'Includes'|t }}: with the colon on the outside or inside for fear of the i18n stuff not passing as well. But worst case I could put both in with an if statement.
Comment #36
star-szrWe should be maintaining the exact same strings within t() calls, so it sounds like an {% if %} in the template will be the way to go.
Comment #37
joelpittetRolled with http://drupal.org/node/1867090#comment-7317770
Still needs #34 With syntax #26
Comment #39
joelpittetOk this still fails those two, but at least http://drupal.org/node/1867090#comment-7317770 is fixing the TwigReference in loop issue!
This is changes from #34
Comment #40
joelpittetOk this should pass the tests now. I removed the theme() call and replaced both with drupal_render(). I know we don't want to use that, but TwigReference will not return false in a condition because it's an array with '#theme' and other keys(ie. not an empty array or rendered empty string). Suggestions here?
Comment #41
star-szrIf you weren't passing the variable to t() later, you could just do something like this - or put the empty array in an 'else' if you prefer.
…but since you pass it to t() later, that won't work. I don't know a way around that - at this time anyway if it goes through t() it needs to be a string. So the way you did it is fine, I can't think of any better way.
Comment #42
joelpittetre-rolled as #1867090: Nested Twig 'for' loops behaving strange landed
Comment #43
tlattimore CreditAttribution: tlattimore commentedThis seems like a lot of @todo's here.
Comment #44
thedavidmeister CreditAttribution: thedavidmeister commented#43 - yeah, that's been happening in a few patches where theme functions were badly/not documented in the original code - especially in some of the Views conversion patches. Not mentioning a variable used in a Twig template is not ok but if we really don't know what each one does in detail, leaving @todo is ok for now.
Comment #44.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdate conversion table
Comment #45
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #46
joelpittetComment #47
webthingee CreditAttribution: webthingee commentedRe Roll from #42
Will be working on the updates next.
Comment #49
webthingee CreditAttribution: webthingee commentedFailed because the link 'Check Manually' was missing on the reports/updates page.
This patch is a reroll of the patch from #42, with the necessary update that allow it to pass testing.
Comment #51
webthingee CreditAttribution: webthingee commentedupdate comments
Comment #52
webthingee CreditAttribution: webthingee commentedThe previous patch did not pass...
This last one is passing all my local simpletests... w00t!
ready for some review!! :P
Comment #53
tlattimore CreditAttribution: tlattimore commentedGreat job on this re-roll @webthingee! Here's a comment of adjustments that need to be made to this:
Could we say something like "..displaying the last time we checked for update data" or something like that?
Need an extra "*" at the beginning of this line.
Indent inside spaceless here.
Indentation needs to be 2 spaces inside
div class="versions"
Indentation needs to be adjusted.
Comment #54
webthingee CreditAttribution: webthingee commentedUpdated with http://drupal.org/node/1898466#comment-7457564, as well as found a typo in one of the form variables.
Checked locally with simpletest... fingers crossed, here it comes again....
Comment #55
ezeedub CreditAttribution: ezeedub commentedPoint the home page to admin/reports/updates
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4463d308ce&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a4463d308ce&...
Comment #55.0
ezeedub CreditAttribution: ezeedub commentedRemove sandbox link
Comment #56
joelpittetProfiling looks great, after the image style stuff get's removed or ... explained maybe needs re-profiling.
not sure how this image module stuff got in here.
Should be project not product. Whoops, I probably did that:S
Comment #57
tlattimore CreditAttribution: tlattimore commentedHere's an updated patch that removes the image related stuff @joelpittet mentioned in #56, also fixes the comment in `update-report.html.twig` as well.
Comment #58
joelpittetFew more patch cleanups and @todo cleanups. Thanks @tlattimore for removing the image stuff and that typo fix.
Couple of 'product' instead of 'project' typos in there... you're welcome:P
Need to find some words to fill these @todos.
This needs to be removed and just the 'version' class should stay.
Couple of 'product' instead of 'project' typos in there... you're welcome:P
Comment #59
siccababes CreditAttribution: siccababes commentedhello i am calling dibs
Comment #60
siccababes CreditAttribution: siccababes commentedI tried to reroll the patch. Hope it works!
Comment #61
siccababes CreditAttribution: siccababes commentedComment #62
siccababes CreditAttribution: siccababes commentedtestbot go!
Comment #63.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #64
jenlamptonrerolled, and took care of most of joelpittet's comments in #58 except for the @todos.
Also:
- removed update_last_check
- update_status_label
These should not be their own templates.
If this fails tests tho I'm happy to move these off into a sub-issue.
Comment #67
pplantinga CreditAttribution: pplantinga commentedHere's an updated patch for #64 that fixes the 2 test fails, plus interdiff.
The {%--%}'s are the result of filter_xss having issues with extra whitespace...
Comment #68
adamcowboy CreditAttribution: adamcowboy commentedThe status report page renders correctly.
Comment #69
jenlamptonun tagging
Comment #70
alexpottIt seems weird that we've replaced a theme table with such explicit markup. How come?
Also this patch conflicts with #2048933: Replace theme() with drupal_render() in update module
Comment #71
siccababes CreditAttribution: siccababes commentedLet's not add this new template. Instead, let's update to a render array with #type = table.
Let's remove this conversion from here and use drupal_render. Also, see https://drupal.org/node/1938934.
Comment #72
joelpittet@alexpott the reason I went this way was the thought that this template doesn't need to be a table at all and in hopes to make it easier for themers to change it to something better. It's definitely overkill and a bunch of work to get it like that but when talking to other themers they seemed to agree that if they wanted to change the template it was a choice of manipulating a large renderable array or changing a twig template it would be much more practical to do the latter.
The flip side to the coin though is that if this never get's converted to divs there is really no point to theme_update_report and converting to #type=>table or whatever would be the way it would go... so probably just me dreaming;)
Comment #73
drupalmonkey CreditAttribution: drupalmonkey commentedPatch rerolled.
Comment #74
drupalmonkey CreditAttribution: drupalmonkey commentedIncluded a patch to get a module and theme to show up on the updates page. To use it:
Install Nivo Slider module version 8.x-1.3 to the DRUPAL_ROOT/modules/ directory:
drush dl nivo_slider-8.x-1.3
Install the Red theme version 8.x-1.0-alpha1 to the DRUPAL_ROOT/themes/ directory:
drush dl red-8.x-1.0-alpha1
Apply the update-testing-patch.txt file. Load: admin/reports/updates
Comment #75
joelpittetThank you for the cleanup @drupalmonkey! We should probably do another round on manual testing on this using your steps!
Comment #76
star-szrTagging for reroll.
Comment #77
pplantinga CreditAttribution: pplantinga commentedReroll
Comment #78
joelpittetThanks for the re-roll @pplantinga!
Minor nitpicks.
These @see template_preprocess() can be removed.
This block is indented one indent too far.
I'm more and more convinced that this approach of building the tables in the twig file is better for DX, better for themers to replace later and has some performance over #type=>table. This also keeps to my goal of having less to no markup in PHP.
Comment #79
richardj CreditAttribution: richardj commentedComment #80
richardj CreditAttribution: richardj commented- Removed @see template_preprocess
- Fixed indenting
Comment #82
richardj CreditAttribution: richardj commented#80: twig-update_conversion-1898466-80.patch queued for re-testing.
Comment #84
richardj CreditAttribution: richardj commentedTrying this with a new patch (very new to interdiffing), same changes as in #80
Comment #85
pplantinga CreditAttribution: pplantinga commentedAnother shot at it.
Comment #86
pplantinga CreditAttribution: pplantinga commentedWhoops looks like we posted at the same time. Yours looks more like the right size than mine...
Comment #87
richardj CreditAttribution: richardj commented#84: twig-update_conversion-1898466-84.patch queued for re-testing.
Comment #88
evanbarter CreditAttribution: evanbarter commentedTook a look at richardj's patch and compared the markup to pre-conversion markup, there was a couple of classes missing, new patch attached.
Comment #88.0
drummhow to test
Comment #89
joelpittet88: twig-update_conversion-1898466-88.patch queued for re-testing.
Comment #90
Nebel54added a follow-up issue to description #2099293: Replace table with available module updates with better markup
Is the remaining task in the issue description still up-to-date? Or is this issue ready for manual testing?
Comment #91
joelpittet@Nebel54 This is ready for manual testing in #88.
Comment #92
joelpittetComment #93
star-szrThis needs a reroll, I'll take care of it.
Comment #94
star-szrHere's a reroll, I'm going to review the patch now.
Comment #95
star-szrHere is a first pass at reviewing the code and docs, I'll see about updating the patch with all or most of these changes.
s/tile/title/
I think this should be "(e.g., dev)".
s/versions/version's/
"within a project" maybe?
We say 'subtheme', not 'sub theme'.
These would all be a great fit for a trans block! https://drupal.org/node/2047135
I see that these are translated in the template but that seems a strange approach, we don't usually put variables through t() and we are trained to put strings like this through t().
Everything underneath "Available variables" is indented 2 spaces too many.
Seems like a good fit for a ternary.
s/Prepare/Prepares/ per https://drupal.org/node/1354#themepreprocess.
s/A/An/
These lines can be combined and the Attribute object doesn't need to be instantiated here as of #1982024: Lazy-load Attribute objects later in the rendering process only if needed.
Comment #96
joelpittetDid all the fixes up in except item 6 in #95. Thanks for the review @Cottser!
I don't think transblock is good for short text like that, it's great for long hunks of text but you're welcome to tackle that if you feel it will suite it better.
Comment #97
star-szrHere's the trans block stuff, I could go either way but I think this reads better. Also wrapping a slightly long comment at 80 chars.
I think we need to pass in the 'class' variable for the attributes here.
Something like this maybe?
Anyway unassigning and back in your court @joelpittet :)
Comment #98
star-szrAlso I think maybe base_themes should be placeholder instead of passthrough here unless there is something special about base_themes.
Comment #99
joelpittetbase_theme was sent through placeholder for the items and then through passthrough, so it is a special case. Thanks for spotting the class was missing. I unioned the arrays, that should do the trick and won't clobber any attributes passed through.
Comment #100
sunSorry for my ignorance, but why are we introducing custom theme templates for those tables here?
There's hardly a case for overriding these tables in any way, so IMO those should simply be built + rendered via #type table, and done.
FWIW, that aspect applies to pretty much all tables. There are use-cases for manipulating classes, injecting/manipulating/removing table rows and columns, which is a typical preprocess/alter task, but otherwise, a table is a table. There's no use-case and thus no point in duplicating the basic HTML output of a table in individual templates.
Comment #101
joelpittetThe main reason we decided this for this table is that it's a single column table with lots of markup inside the cell. The idea was to make it easier for themers to replace the markup with a div or ul list structure that better suited it. Though a Views listing page would allow that too... and may better suit this if that is possible?
Also it's easier for us to get pass the performance gate with this then #type=>table currently as we are required to have each template profiled.
Comment #102
joelpittetMaybe there is a compromise here... I don't want to hand code all the update detail markup in a controller but maybe that whole chunk in twig can be a new theme function and we can replace the table surrounding it with a #type=>table. And update_version would be a straight forward #type=>table. Deal?
Comment #103
sunNot sure whether this is what you meant, but
Can we move the inner markup (of a table cell in the loop; i.e., the output for a single project) into a separate template, so that it is decoupled from the outer/wrapping table?
→ That way, it would be easier to get rid of the wrapping table in a separate follow-up issue. (since I can't see why the wrapping output is a table to begin with)
Make update_version a simple #type table and just supply it as theme variable to aforementioned single project template?
Is that what you meant? :)
Lastly, @webchick recently worked on the port of Upgrade Status module to D7, in which we identified that the module still has to copy/fork a lot of code of Update module in core... Assigning this issue to her, in the hope that she might be able to give some feedback with regard to the template variable preprocessing (→ ideally the Upgrade Status module should be able to re-use this theme template as-is)
Comment #104
joelpittetYes, that is exactly what I meant:)
Comment #105
star-szrComment #106
webchickIf you two are in agreement, sounds good to me. :) It's true that I did port upgrade status, but it was awhile back on weekends in between baby naps. :)
I'm SO sorry for letting this sit so long. :(
Comment #107
joelpittetAssigning to tackle #102/103.
Comment #108
joelpittetNot sure what to do with
update_version
so I just left it as a theme function. Any suggestions for that theme function please let me know, maybe a helper function or just leave it as a theme_function, or maybe extend #type=>table suggestion?.This simplifies the preprocess by splitting the update_report into two templates. There is still some markup in the update-report.html.twig so I left the twig file for that.
Comment #109
joelpittetDeal with 'theme_update_version'. I'm thinking private helper function is the way to go because it's just helping build a #type=>table now and likely doesn't need to drupal_render it.
Comment #110
star-szrTagging for reroll.
Comment #111
joelpittetRe-rolled.
Comment #112
rodrigoaguileraThe status report is rendering right for me
Comment #113
webchick111: 1898466-twig-update-new-reroll-111.patch queued for re-testing.
Comment #115
rodrigoaguilerarerol
Comment #116
rodrigoaguileraplease review since I'm learning here at #drupaldevdays
Comment #117
joelpittetWe still have this theme_ function in there... not sure what to do with it yet... But maybe we can deal with it in a follow-up issue as this still get's rid of the markup that it was writing in strings in this. And all the other theme conversions?
Thanks for the re-roll @rodrigoaguilera!
Assigning to @webchick to see if that would be cool/kosher?
Comment #119
joelpittet115: 1898466-twig-update-new-reroll-115.patch queued for re-testing.
Comment #121
webchickI think given how massive this patch is already, it's fine to split that off into a separate issue. OTOH if it's easy to knock out, let's do it here. :)
Comment #122
chrisfromredfinI think this fail was a fleeting issue with the testbot so marking back to needs-review.
Comment #123
joelpittetI'm planning to split off all these templates one by one because the re-roll is complicated and it's a big patch.
Here is #2264753: Convert theme_update_last_check to update-last-check.html.twig the first for theme_update_last_check, which was removed, but that may be too soon and can be consolidated later.
Comment #124
bdragon CreditAttribution: bdragon commentedquickie reroll just in case because I was poking around this stuff for chx sandbox autoescape reasons.
Comment #125
bdragon CreditAttribution: bdragon commentedaw hell, was working off the -99 patch. Please disregard that one.
In any case, it would be nice to have this stuff in because it will fix some of the breakage in the chx sandbox autoescape branch...
Comment #126
bdragon CreditAttribution: bdragon commentedReroll correct patch. Only change is fuzz removal...
Comment #127
hass CreditAttribution: hass commentedThere are several strings that need to be ucfirst. These are: warning, error, ok
Comment #128
joelpittet@hass, this is a conversion issue. If that needs to happen it can be in a followup/separate issue. The before and after for manual testing should be identical in a conversion issue.
Comment #129
hass CreditAttribution: hass commentedWe convert D7 to D8, isn't it? This ucfirst is a D8 rule.
Comment #130
joelpittetThis is not D7 to D8 conversion. This is theme_* function to twig template conversion. For this type of issue, we try to avoid re-rolls by not doing scope changes that maybe applicable to other D8 issues as much as possible. Mostly because it effects manual testing and also may have an effect on xhprof profiling results as well.
When you create a followup could you also point in the issue summary of the new issue where ucfirst is a rule so we can point people to that standards change?
Comment #131
hass CreditAttribution: hass commentedComment #132
star-szrTagging for another reroll.
Comment #133
lokapujyaRerolled.
Comment #134
lokapujyaReroll. I messed up on a conflict.
Comment #135
lokapujyaAttaching the interdiff. Accidentally attached the previous patch instead of the interdiff to the last post.
Comment #137
lokapujyaI don't think there is a coding standard set, but = &$ is usually preferred.
The image paths have changed.
Comment #139
joelpittet@lokapujya you are totally right on the referenced variable convention, there is 34 of the way I did it and 168 of the way you mentioned.
= &$
wins:)Thanks for catching that image patch change, that will need to go.
I've split out theme_update_version into it's own contentious issue: #2264833: Convert theme_update_version to update-version.html.twig So it no longer needs to live in this patch.
I'd like to maybe move some of the other theme functions to their own issue as well to slim jim this patch down a bit for easier review. Thoughts?
Comment #140
xjmIn #1825952: Turn on twig autoescape by default, in
theme_update_report()
, we are marking the$data
array of project status (wrapped in a<p>
tag) as safe. Once that goes in, we should check with whatever is providing the$data
variable to ensure that we don't run into double-escaping. Adding a reference to this issue in that patch.Comment #141
steveoliver CreditAttribution: steveoliver commentedAdding issue relationship to #1825952: Turn on twig autoescape by default.
Comment #142
l0keReroll of 134 patch, considered the notes above.
Comment #144
l0keMessed up a little in previous patch, fix exceptions.
Comment #145
star-szrThanks @lokeoke, great to add interdiffs when updating patches, as a rule upload them every time you change a patch, unless you are doing a reroll. Thanks again :D
Comment #146
l0keHere is interdiff, i messed up with an Attribute objects and didn't remove theme_update_status_label when rerolling the patch.
Comment #147
joelpittet@lokeoke That's awsome thank you! Here is a few more changes from that. I did a manual test which seemed to look good but probably needs a few more scenarios added to the issue summary because by default you only get that Drupal Core is out of date at the moment.
Also I added a nice syntax sugar in twig the for..else
I so wanted to use it and seems like a nice place to try it out.
http://twig.sensiolabs.org/doc/tags/for.html#the-else-clause
Comment #148
joelpittetForgot some p tags after testing the no updates works. And changed the variable name because it didn't make much sense before.
Comment #149
joelpittetHere's a round of manual testing. The markup doesn't change.
I normalized the whitespace for comparison and uploading both versions. (norm = normalize;)
Noticed the images are broken but that is broken before this patch too so just a note and I'll likely need to open up a new issue if there isn't one already.
Comment #150
steveoliver CreditAttribution: steveoliver commentedReviewing this in the next hour or so.
Comment #151
steveoliver CreditAttribution: steveoliver commentedManual testing looks good -- one extra whitespace in output seems worth it for a clean template.
Attached patch addresses these points:
1. Available variables, minor grammar:
- inludes: "The projects within a project." ==> "The projects within the project."
- disabled: "The disabled included projects." ==> "The currently disabled projects in the project."
- base_themes: "The base theme declared for the project." ==> "The base themes supplied by the project."
- sub_themes: "The subthemes declared for the project." ==> "The subthemes supplied by the project."
2.
|e
escape filters can be removed here because of #1825952: Turn on twig autoescape by default3. Since it looks strange, add a comment about why we use
|passthrough
instead of|placeholder
here?@param $variables
not finished.$status
variable in template_preprocess_update_report().Comment #152
steveoliver CreditAttribution: steveoliver commentedSince all the changes in my last review-patch are minor grammar changes and removal of unneeded
|escape
filters, I think this is ready to go! Nice work, everyone! (also linked this to the broken images issue).Comment #153
alexpottCommitted c2e66f9 and pushed to 8.x. Thanks!
Fixed on commit.
Comment #156
m1r1k CreditAttribution: m1r1k commented