Issue #2151097 by joelpittet, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Convert theme_confirm_form() to Twig

Task

Convert theme_confirm_form() to a Twig template.

Remaining tasks

  • Patch
  • Patch review
  • Manual testing
  • Profiling

Steps to test

@todo

CommentFileSizeAuthor
#9 screenshot_1.png29.18 KBjjcarrion
#8 confirm_form_after_new_patch.png45.27 KBchakrapani
#8 confirm_form_before_patch.png45.27 KBchakrapani
#8 convert_theme_confirm_form_to_twig-2151097-8.patch1.56 KBchakrapani
#2 2151097-convert-theme-confirm-form-2.patch1.6 KBrteijeiro
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

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

Adding a commit message to the issue summary so the folks who already worked on #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.

rteijeiro’s picture

rteijeiro CreditAttribution: rteijeiro commented
Status: Active » Needs review
FileSize
2151097-convert-theme-confirm-form-2.patch1.6 KB

Let's see but I'm not sure what I'm doing :(

star-szr’s picture

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

It would be nice if this was a suggestion like form__confirm_form.

MartiMcFlight’s picture

MartiMcFlight CreditAttribution: MartiMcFlight commented
Assigned: Unassigned » MartiMcFlight
Issue tags: +SprintWeekend2014, +D8MA
MartiMcFlight’s picture

MartiMcFlight CreditAttribution: MartiMcFlight commented
Assigned: MartiMcFlight » Unassigned

Sorry i don‘t know how to implement what Cottser requests in comment #3 …

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +Twig conversion
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work
Issue tags: +Novice

Some documentation fixes needed for this. If there is a decent way to not even have this template I'm open for suggestions, but otherwise lets just touch the docs up and get a manual testing.

  1. +++ b/core/modules/system/system.module
@@ -2849,22 +2850,6 @@ function system_run_automated_cron() {
- * By default this does not alter the appearance of a form at all,
- * but is provided as a convenience for themers.

    This should be added in some respect to the twig template. 

  2. +++ b/core/modules/system/templates/confirm-form.html.twig
@@ -0,0 +1,16 @@
+ * Template for confirm form..

    Two periods and should follow convention for the docblock titles in https://drupal.org/node/1823416#docblock 

  3. +++ b/core/modules/system/templates/confirm-form.html.twig
@@ -0,0 +1,16 @@
+ * This template will be used when a confirm form specifies 'config_form'
+ * as its #theme callback.  Otherwise, by default, confirm forms will be
+ * themed by theme_form().

    Config forms do this by default, not sure the description gives any any needed information and probably just be replaced by the theme function note about the form appearence.

chakrapani’s picture

chakrapani CreditAttribution: chakrapani commented
Status: Needs work » Needs review
FileSize
convert_theme_confirm_form_to_twig-2151097-8.patch1.56 KB
confirm_form_before_patch.png45.27 KB
confirm_form_after_new_patch.png45.27 KB

Here we go. I have added a patch with the changes mentioned in #7.
Also did manual testing and uploading screenshots before and after applying the patch.
I have also verified the HTML source and found no difference.

Deleting a node before aplying patch
Deleting a node after aplying patch

jjcarrion’s picture

jjcarrion
Primary language Spanish
Location Spain
CreditAttribution: jjcarrion commented
Status: Needs review » Reviewed & tested by the community
FileSize
screenshot_1.png29.18 KB

The patch #8 works as expected. I attach a screenshot

galooph’s picture

galooph CreditAttribution: galooph commented

The patch from #8 applied cleanly and worked as expected.

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!

  • Commit f8ec2d8 on 8.x by webchick: 
    Issue #2151097 by rteijeiro , chakrapani , joelpittet, c4rl, IshaDakota...

Status: Fixed » Closed (fixed)

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