Suggested commit message:

Issue #1885564 by Cottser, SebCorbin, joelpittet, drupalninja99, jenlampton, longwave, aboros, trevorkjorlien, socketwench, shanethehat, mbrett5062, rteijeiro: Convert theme_task_list to Twig template.

Split from #1885564: theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig

Task

Convert theme_task_list() to a Twig template

Remaining

  • Patch needs review
  • Manual testing needed - particularly for authorize.php

Manual testing steps

  • task-list.html.twig - visit update.php, look for list in left sidebar under Druplicon - "Verify requirements", "Overview", etc.

#1885714: Remove theme_install_page()
#1885850: Remove theme_update_page()
#1189822: Convert maintenance-page.html.twig to HTML5

CommentFileSizeAuthor
#14 interdiff.txt1.56 KBstar-szr
#14 2329505-14.patch3.34 KBstar-szr
#12 2329505-12.patch2.64 KBstar-szr
#9 interdiff.txt549 bytesstar-szr
#9 2329505-9.patch3.71 KBstar-szr
#7 update-page.png310.82 KBrteijeiro
#4 interdiff.txt1.72 KBjoelpittet
#3 interdiff.txt2 KBjoelpittet
#3 2329505-twig-theme_task_list-4.patch3.71 KBjoelpittet
#2 interdiff.txt2 KBjoelpittet
#2 2329505-twig-theme_task_list-2.patch3.18 KBjoelpittet
#1 2329505-twig-theme_task_list-1.patch3.18 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Active » Needs review
FileSize
2329505-twig-theme_task_list-1.patch3.18 KB

Here's the split.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
2329505-twig-theme_task_list-2.patch3.18 KB
interdiff.txt2 KB

Cleanup of the preprocess and markup update to match changes in HEAD, remove parens and move space inside the span.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
2329505-twig-theme_task_list-4.patch3.71 KB
interdiff.txt2 KB

Whoops, forgot the hook_theme() in both the above and added the wrong class, and those parenthesis are needed.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
interdiff.txt1.72 KB

And wrong interdiff..argh... sorry... again.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented 
+++ b/core/modules/system/templates/task-list.html.twig
@@ -19,7 +19,7 @@
+    {% if task.status %}<span class="visually-hidden"> ({{ task.status }})</span>{% endif %}

Is this extra space inside the span needed?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented

It was there before, so it might not be needed but we should preserve it for now to limit the scope of this to converting to Twig.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Needs review » Reviewed & tested by the community
FileSize
update-page.png310.82 KB

Ok, then it's a RTBC. Update page works as expected and code looks good.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -Needs manual testing

Great, thanks @rteijeiro :D

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes
FileSize
2329505-9.patch3.71 KB
interdiff.txt549 bytes

One line tweak, and adding a suggested commit message to the issue summary (from the parent issue).

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes

joelpittet++

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I'm almost certain this will need a reroll after #2250119: Run updates in a full environment.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2329505-12.patch2.64 KB

No major changes, it turns out #2250119: Run updates in a full environment just moved the theme function to theme.inc. That means the 'include' line in drupal_common_theme() should no longer be needed, so I removed it.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

@Cottser #12 seems to be missing the theme function removal.

Thanks for the re-roll.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs work » Needs review
FileSize
2329505-14.patch3.34 KB
interdiff.txt1.56 KB

D'oh. Will cancel the last one. Thanks @joelpittet!

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Thank you:) Back to RTBC as per #7

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Also, this would be out of scope of this issue, which is just a conversion, but there's no way I would've associated a template called "task-list.html.twig" with "a list of maintenance tasks to perform." Maybe we could discuss this in a follow-up?

  • webchick committed f92c2cb on 8.0.x 
    Issue #2329505 by Cottser, SebCorbin, joelpittet, drupalninja99,...
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@webchick done #2335003: Rename task-list.html.twig to maintenance-task-list.html.twig

Thanks for committing:)

Status: Fixed » Closed (fixed)

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