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.
Task
Convert form.inc theme_ functions to Twig templates.
Remaining
- #2152219: Convert theme_input() to Twig
- #2152201: Convert theme_checkboxes() to Twig
- #2152205: Convert theme_date() to #theme input__date
- #2152215: Convert theme_form_element_label() to Twig
- #2152217: Remove theme_form_required_marker() from the theme system - use CSS instead
- #2152221: Convert theme_radios() to Twig
- #2152227: Convert theme_tableselect() to #theme table__tableselect
- #2152229: Convert theme_textarea() to Twig
- #2152231: Convert theme_vertical_tabs() to Twig [small followup]
- #2152203: Convert theme_container() to Twig
- #2152207: Convert theme_details() to Twig
- #2152209: Convert theme_fieldset() to Twig
- #2152211: Convert theme_form() to Twig
- #2152225: Convert theme_select() to Twig
- #2152213: Convert theme_form_element() to Twig
Comment | File | Size | Author |
---|
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
c4rl CreditAttribution: c4rl commentedBased on #1905584: Move base theme system templates into /core/templates it seems this issue would be accommodated by #1898454: system.module - Convert PHPTemplate templates to Twig, though that may result in a large patch.
Comment #3
c4rl CreditAttribution: c4rl commentedIgnore what I said about system.module in #2, we'll proceed to convert this separately of system.module and put in /core/templates as described in #1905584: Move base theme system templates into /core/templates
Comment #4
steveoliver CreditAttribution: steveoliver commentedI'll tackle this.
Comment #5
steveoliver CreditAttribution: steveoliver commentedWork in progress. Just posting in case I don't get to this soon and/or someone else reeeallly wants to work on it. I'd rather finish it though, time permitting, since I've been working on it so far.
Of note is the idea that, like in #1812724-10: Consolidate all form element templates and add theme_hook_suggestons, I'm trying to drive a separation between Render/Form API and Theme APIs by having the preprocess/theme functions handle generic element rendering while Form API stays responsible for FAPI-specific things like tableselect, etc. More on that later.
Comment #7
steveoliver CreditAttribution: steveoliver commentedOh, that's right, there's no theme engine at install time...
Comment #8
steveoliver CreditAttribution: steveoliver commentedAlright, let's see. *Includes patch from #1942490: Make Twig engine available during install.
Comment #10
steveoliver CreditAttribution: steveoliver commentedShould fix at least some fails for textareas.
Comment #10.0
steveoliver CreditAttribution: steveoliver commentedAdded git attribution code.
Comment #12
steveoliver CreditAttribution: steveoliver commentedThis (hacky) should take care of some form fails.
Comment #14
steveoliver CreditAttribution: steveoliver commentedOK, going back on the #pre_render stuff because I can't figure out why/where node forms /don't/ seem to use #type 'form'. Maybe less than 10 fails? Still don't know what the cache issue is about.
Comment #16
steveoliver CreditAttribution: steveoliver commentedThis patch should fix all the exceptions. Still can't figure out that 1 fail. Translation seems to not be getting saved.
Comment #18
steveoliver CreditAttribution: steveoliver commentedI'd love someone to take a look at this patch and help me get that one last Locale test to pass.
I tested without changing theme_textarea or theme_input, and still no go. I didn't test without theme_form*, so that might be the next place to look. Unassigning until I can get back onto this or someone else can have a look.
Comment #19
steveoliver CreditAttribution: steveoliver commentedtheme_form_element_label() conversion is what causes the 1 fail above. What's strange is
1. The test seems to be testing successful form submission, which should be unaffected by labels (and is, as I removed the label altogether and the test passed)
2. The actual form element label tests pass for the "form-element-label.patch".
So the this main patch should pass, which excludes the form element label conversion.
The label patch needs work, but I can't tell what... What I do know is, based on the test output, the string actually isn't translated. The question in my mind now is Why the heck would that test fail based on a form element label. If anything I'd expect issues from form, submit, etc...
Comment #21
steveoliver CreditAttribution: steveoliver commentedDrum roll, please... (this should be green).
Comment #23
steveoliver CreditAttribution: steveoliver commentedWoops, forgot the install.core.inc patch (necessary until #1942490: Make Twig engine available during install lands). Here it is
Comment #25
steveoliver CreditAttribution: steveoliver commentedThrashmaster! Need to figure out how to not have to do this:
Comment #27
steveoliver CreditAttribution: steveoliver commentedI /will/ get this thing green... I /will/ get this thing green...
Comment #28
steveoliver CreditAttribution: steveoliver commentedAdded whitespace modifier dashes to get label tags and contents all on one line. Should fix those two fails. Show me green!
Comment #29
steveoliver CreditAttribution: steveoliver commentedOK, after all that thrashing, here's an overview of what's going on in this patch and a few comments on pieces of the code.
What this does, besides straight conversion:
Here are some comments on specific pieces of code:
In the comments change here and throughout this patch I'm trying to decouple and clarify the relationship between Form and Theme APIs.
This removes theme_tableselect(). #type 'tableselect' element now does its preparation in it's own #pre_render and uses #theme 'table__tableselect'. Again, separating Forms/element handling from HTML element themeing.
The addition of
\s*?
is required because, since Twig (PHP) files need a trailing newline, the form-label-required-marker.html.twig produces a newline within the label. I saw no other way around this, and seems fine to me.I'm honestly not sure why I needed to change these from #theme to #theme_wrappers, but I believe this was the only thing that worked.
This template existed before this form.inc conversion. I'm just cleaning up the comment and attributes.
Woops, I need a newline here.
This render array (above)....
...requires me to target the element like this, instead of just $variables['element'], which I'm pretty sure is what we want to be doing. Could use an extra set of eyes here.
Comment #30
steveoliver CreditAttribution: steveoliver commentedRe #29, point #4: This doesn't actually get us all the way to being able to use those theme functions without knowing about a render element in preprocess, so maybe I'll post another patch that pulls $element knowledge out of the preprocess functions and all the way into #pre_render.
Comment #31
joelpittet@steveoliver merged your change for the datetime doc into the datetime twig conversion
#1939080: Convert theme_datetime() to Twig
If it doesn't affect this patch you could take it out.
type=>tableselect is already on the chopping block @see #1876714: Remove #type 'tableselect' but it's waiting on the type=>table conversion @see #1876712: [meta] Convert all tables in core to new #type 'table' to be completed. So may be a bit early to drop it because it's used in locale, comment, system, rest, update modules and a few tests.
small typo ending period instead of comma:
+ * Default template: select.html.twig,
Not sure about the backtick
`
if that was intended or not? @seeform_pre_render_date
Otherwise great work!
Comment #32
steveoliver CreditAttribution: steveoliver commentedRe-roll without installer part, in light of #1942490: Make Twig engine available during install.
Comment #33
star-szrAgreed with #31, let's make it so this patch doesn't touch datetime.html.twig. See also #1898454-13: system.module - Convert PHPTemplate templates to Twig.
Fantastic work on this by the way, @steveoliver.
Comment #34
steveoliver CreditAttribution: steveoliver commentedIncorporating #31 and #33.
Comment #35
steveoliver CreditAttribution: steveoliver commentedWoops-- accidentally included all kinds of other stuff -- looks like I forgot to rebase... new patch coming up
Comment #36
steveoliver CreditAttribution: steveoliver commentedThis should do it.
Comment #38
star-szr#36: drupal-twig-forminc--1898480-36.patch queued for re-testing.
Comment #39
star-szrIt looks like the only conversion missing in this issue is theme_exposed_filters(), and it looks like it has been converted in the sandbox already. Great work @steveoliver :)
Comment #40
star-szrAlso theme_exposed_filters() is weird.
The theme function is defined in drupal_common_theme() in theme.inc under the form.inc heading, but the theme function itself is in system.module. So we should remove the theme function from system.module but add the preprocess function (if necessary) to form.inc.
Comment #40.0
star-szrFormatting.
Comment #41
steveoliver CreditAttribution: steveoliver commented...something like this...
Comment #42
star-szr@steveoliver is that for #1912600: Remove theme_views_form_views_form in favour of a prerender callback. perhaps?
Comment #42.0
star-szrAdd conversion summary table
Comment #43
star-szrTagging.
Comment #44
star-szrGot the okay from @steveoliver to unassign for the remaining conversion.
Bringing over the theme_exposed_filters() conversion from the sandbox would be a good Novice task. I recommend taking a look at @c4rl's screencast: http://www.youtube.com/watch?v=HS4yKJjrb2E
Comment #45
shanethehat CreditAttribution: shanethehat commentedComment #46
shanethehat CreditAttribution: shanethehat commentedComment #47
star-szrThat's great, thanks @shanethehat. The below items can be done to move this issue forward:
"Prepares" instead of "Preprocess" and @param array $variables please per #1913208: [policy] Standardize template preprocess function documentation.
We should try and remove the drupal_render() and theme() calls here per http://drupal.org/node/1920746.
For $variables['items'] you should be able to just treat it as an array, i.e:
And set $variables['items'] to an empty array instead of an empty string before the if statement.
We should unindent the available variables list here per http://drupal.org/node/1354#lists, and document the 'items' variable that is being added in the preprocess function.
textarea.html.twig needs a blank line at the end of the file per http://drupal.org/coding-standards#indenting.
And a note for other reviewers:
I think this is fine for now, we are working on getting rid of these calls in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead but for conversion purposes this works. Since this doesn't need special handling like #1898432: node.module - Convert PHPTemplate templates to Twig I don't think we need to add a @todo here.
Comment #47.0
star-szrUpdate summary table
Comment #48
shanethehat CreditAttribution: shanethehat commentedComment #49
shanethehat CreditAttribution: shanethehat commentedComment #50
shanethehat CreditAttribution: shanethehat commentedThis triggers a whitespace error when applied, but I think it's caused by #47 point 4.
Comment #51
star-szrExcellent work @shanethehat!
The removal of drupal_render() and conversion to a render array look good here.
Minor coding standards nitpicks:
The 'class' line should be indented by 2 spaces (http://drupal.org/coding-standards#indenting), and these array items need a trailing comma per http://drupal.org/coding-standards#array.
This needs a blank line with only
*
between the variables and the two @see lines.We are also trying to avoid using the word 'array' in template files, if you can reword these to whatever you think makes sense that would be great. If you're not sure just ping me :)
@shanethehat - if you want to go through the preprocess function docblocks and Twig template docblocks updated/added in this patch and rework them as best as you can to match the standards that would be a huge help.
Relevant links for reference:
#1913208: [policy] Standardize template preprocess function documentation
Twig coding standards
Comment #52
shanethehat CreditAttribution: shanethehat commentedComment #53
shanethehat CreditAttribution: shanethehat commentedComment #54
shanethehat CreditAttribution: shanethehat commentedStupid whitespace fail
Comment #54.0
shanethehat CreditAttribution: shanethehat commentedUpdate commit message
Comment #54.1
star-szrUpdate conversion table
Comment #55
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 #56
intergalactic overlords CreditAttribution: intergalactic overlords commented#54: drupal-twig-forminc--1898480-54.patch queued for re-testing.
Comment #58
Hanpersand CreditAttribution: Hanpersand commented#54: drupal-twig-forminc--1898480-54.patch queued for re-testing.
Comment #60
Hanpersand CreditAttribution: Hanpersand commentedGoing to re-roll this.
Comment #61
Hanpersand CreditAttribution: Hanpersand commentedRe-rolled!
Comment #62
socketwench CreditAttribution: socketwench commentedWorks for me! RTBC!
Comment #64
Hanpersand CreditAttribution: Hanpersand commented#61: drupal-twig-forminc--1898480-61.patch queued for re-testing.
Comment #66
Hanpersand CreditAttribution: Hanpersand commentedThis passed for me and @socketwench at the DrupalCon PDX sprint in both our local testing environments, but the bot failed it on BlockRenderOrderTest.php.
Comment #67
star-szrThanks @hanpersand and @socketwench :) See #1987952-15: Blocks are not rendered in order by weight for an explanation of the test failures, unrelated.
Comment #68
star-szr#61: drupal-twig-forminc--1898480-61.patch queued for re-testing.
Comment #69
star-szrBTW, found the other issue by running
git log -S BlockRenderOrderTest
after I couldn't find the test locally.Comment #70
joelpittetReviewing the code we have a few improvements to make because of other issues related to performance. See below:
All of the
+ $variables['attributes'] = new Attribute($variables['element']['#attributes']);
in this patch should be replaced with
+ $variables['attributes'] = $variables['element']['#attributes'];
due to #1982024: Lazy-load Attribute objects later in the rendering process only if needed
The drupal_render_children() is no longer needed due to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead
Comment #71
dsdeiz CreditAttribution: dsdeiz commentedPer #70, I've removed
new Attribute(...)
from all 'attributes' and 'title_attributes' (didn't find 'content_attributes' though). Also removeddrupal_render_children()
but not entirely sure if I got it right.Comment #72
dsdeiz CreditAttribution: dsdeiz commentedComment #74
joelpittetHere's a few changes on the last patch to help it pass and a bit more nitpiks.
@dsdeiz thank you vrery much for the patch, if you want some details on what's going on there jump on IRC and ping me in #drupal-twig.
Doesn't need an empty array, if it's not set it won't get instantiated later, which is good:)
same
needs an ending comma
Either $variables['children'] = drupal_render_children($variables['element']); needs to be
$variables['children'] = $variables['element'];
Or in twig it needs to be {{ children }}
No need to initialize the array.
Comment #75
joelpittet#71 needs a re-roll and #74 changes
Comment #76
hussainwebWorking on the changes...
Comment #77
hussainwebAttaching the rerolled patch.
@joelpittet, in #74, I assume you meant that the twig file (input.html.twig) should contain
{{ element }}
instead of{{ children }}
. The new relevant portion of code in input.html.twig is:Comment #79
hussainweb#77: drupal-twig-forminc--1898480-77.patch queued for re-testing.
Comment #81
hussainwebFixing the issue with installation. I also tested manually that the installation works. The problem was with a lingering call to get_t().
Comment #83
hussainwebI just tested for the failing test locally and it passed. I even manually verified the count of disabled elements. Queueing for retest.UPDATE: I mixed up the testing instance. I will try manual testing again...
Comment #84
hussainweb#81: drupal-twig-forminc--1898480-81.patch queued for re-testing.
Comment #86
hussainwebI am just rerolling the patch, also taking in account #2031341: Move theme_container into theme.inc, which moved one of the functions to core/includes/theme.inc. I just want to see what testbots think.
Comment #88
hussainwebOk, it seems to be the same failure. The problem is with this test:
The test expects to find exactly 39 disabled elements. However, with the change in the patch, there are 41. The two extra disabled elements are <div> wrappers to
checkboxes
andradios
elements.The reason is this twig file (it is same for both radios.twig.html and checkboxes.twig.html:
This brings the attribute #disabled to the wrapping <div> element. This increases the total count of disabled elements.
These are the possible fixes:
template_preprocess_radios
andtemplate_preprocess_checkboxes
.I will try coming up with a patch for the third method, but I am not sure what could be other effects. Any suggestions?
Comment #89
hussainwebHere is the fix as per approach 3 from #88. I did some manual testing and it looks fine. Let's see the testbot's findings.
Comment #90
jenlamptonCan we get some results from the manual testing posted here? Screen shots or copy/paste of the source code.
Also, I think dreditor removed some tags.
Comment #91
star-szrNeeds another reroll. Thanks for your work on this issue @hussainweb!
Comment #92
hussainwebThanks for the mention!
Here is the reroll. Fortunately, git did the whole job. I didn't have to manually resolve anything.
Comment #93
hussainwebComment #95
joelpittet@hussainweb thanks for the re-roll. I think the fail is whitespace related.
I cleaned up some nitpicky items, and hopefully got the profiling attributes in a better place for form_element.
FYI, for the whitespace I added a comment control to the end to remove the newline character that EOF wants. Any control will do, I just added a comment because it should be the least process intensive.
Comment #96
thedavidmeister CreditAttribution: thedavidmeister commentedthere's still a reference to:
- theme_fieldset() in core/modules/system/tests/modules/design_test/form/fieldset.inc
- theme_details() in core/modules/system/tests/modules/design_test/form/details.inc
- theme_tableselect() in core/includes/form.inc
- theme_form_element() in core/includes/form.inc and core/modules/system/system.api.php
- theme_form_element_label() in core/modules/system/system.api.php
Preapres an element's children details for vertical tabs templates.
Prepares.
these still have @themeable on it, but shouldn't.
- template_preprocess_container()
- template_preprocess_form()
why does vertical-tabs.html.twig not have the same @todo as radios and checkboxes?
$variables['value'] = check_plain($element['#value']);
should be String::checkPlain()
+ $variables['label'] = (isset($element['#title'])) ? filter_xss_admin($element['#title']) : NULL;
should be Xss::filterAdmin();
Comment #97
hussainwebAttaching changes from the review...
Comment #99
hussainweb#97: form-twig-1898480-97.patch queued for re-testing.
Comment #101
hussainwebThe test has broken due to swapped order of HTML attributes. The test is expecting:
<label class="visually-hidden" for="edit-langcodes-de">Update German</label>
but it is generated as:
<label for="edit-langcodes-de" class="visually-hidden">Update German</label>
They are the same thing and I can as well update the test case. It was bound to happen as the label template has changed. I was wondering if there was a better way to test for this so that this is future-proof. Any thoughts?
Comment #102
star-szr@hussainweb - in some cases we've converted tests to use XPath instead of string matching, I can't find an example offhand but that might be reasonable to do here.
Thanks for your work on this issue :)
Comment #103
hussainweb@Cottser, I was thinking along those lines. I just couldn't think of an example. I will look again tomorrow, if possible, but if you find a sample, please let me know.
Comment #104
star-szrIf you grep for '$this->xpath' that should find you many examples :)
API docs here: https://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dr...
Comment #105
joelpittetThis could use manual testing if it passes.
@hussainweb have a look at the test, change in the interdiff, it's quite handy to be able to use xpath to find tags.
Comment #106
joelpittetI think this got added in by accident.
Change the test, add \n
change the test.
Let's get this todo out and move this to 'container__radios'
We should change the test to add the whitespace on here.
Can we consolidate this and remove the theme function for 'container__vertical_tabs'?
Comment #107
joelpittetfrom #106 I did item 1, 2, 3 and 5. And left the container__ bit alone because I didn't get very far with those conversions. Anybody is welcome to tackle those.
Comment #108
joelpittetAnother review. Has the remaining items from #106
Is this possible to fix?
Should this be a follow up issue?
#theme => container__checkboxes?
#theme => container__radios?
#theme => container__vertical_tabs?
Comment #108.0
joelpittetUpdate remaining, remove sandbox link
Comment #109
joelpittet107: 1898480-107-twig-form.patch queued for re-testing.
Comment #111
star-szrJust a reroll and incorporated a change from #2089631: Showing/hiding of CKEditor plugin settings is fragile, automate it (breaks in narrow viewports and when enabling CKEditor) for vertical tabs so they can have attributes passed in.
Probably a terrible idea but I'm seeing what happens when you hide all the patches but the latest from the issue summary.
Comment #113
star-szrHere's a reroll in preparation for splitting this into sub-issues to make this more manageable.
Mostly an update for #2121775: Make the markup associated with the required star on field items silent and #2087239: Remove theme_exposed_filters().
Comment #114
star-szrAdding back the change to \Drupal\system\Tests\Form\FormTest::testRequiredFields().
Comment #115
star-szrComment #118
star-szrConverting to a meta.
Comment #119
joelpittet.
Comment #120
joelpittetShuffling.
Comment #121
star-szrSuper awesome, only #2152227: Convert theme_tableselect() to #theme table__tableselect left now! Meta-ing this really helped I think :)
Comment #122
star-szrAll child issues now fixed, go team! :)