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 #2152229 by steveoliver, rteijeiro, joelpittet, JeroenT, InternetDevels, michamilz, burgerboydaddy, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_textarea() to Twig
Task
Convert theme_textarea() to a Twig template.
Remaining tasks
- Patch
- Patch review
- Manual testing
- Profiling
Steps to test
@todo
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff-2152229-15-28.txt | 503 bytes | InternetDevels |
#28 | convert-theme-textarea-2152229-28.patch | 2.69 KB | InternetDevels |
Comments
Comment #1
star-szrAdding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.
Comment #2
rteijeiro CreditAttribution: rteijeiro commentedNot sure about including element into twig template. Any ideas?
Comment #4
joelpittetYou can remove the space before the
{{ attributes }}
That's may be the only issue, or the array_merge may be clobbering something...
Comment #5
rteijeiro CreditAttribution: rteijeiro commentedRemoved whitespace as joelpittet suggested in #4. Let's see if now it works.
Comment #6
joelpittetOh this need to be wrapped in new Attribute()... a good reason we shouldn't have magic wrapped special case variable keys #2108771: Remove special cased title_attributes and content_attributes for Attribute creation
Comment #8
rteijeiro CreditAttribution: rteijeiro commentedNot sure if I understood it well. Let's see :)
Comment #10
star-szrInterdiff next time please :)
You got rid of the exceptions! And 2 less fails.
Comment #11
joelpittetI'm quite sure it has to do with the change in how the attributes are being created. I don't know what _form_set_attributes() really does but glad to see it gone as I doubt it's needed. Though it would be worth checking those failed tests to see what the diff is between the attributes. A debug() call in those tests will help.
Comment #12
JeroenTI think _form_set_attributes is needed, because it also adds a required class and other stuff when necessary.
Comment #13
JeroenTComment #14
JeroenTFixed wrong indentation.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedAdded a missing space.
Comment #16
burgerboydaddy CreditAttribution: burgerboydaddy commentedwrong test; sorry
Comment #17
burgerboydaddy CreditAttribution: burgerboydaddy commentedComment #18
burgerboydaddy CreditAttribution: burgerboydaddy commentedComment #19
burgerboydaddy CreditAttribution: burgerboydaddy commentedhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e436ce9c71d&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52e436ce9c71d&...
Scenario for this profiling:
0. setup Seven 8.0-dev as default theme
1. added additional field to article content type.
2. created node as article type.
3. setup page as front page
4. allowed anonymous user to view, post, edit, and skip comment approval
5. run tests
Comment #20
burgerboydaddy CreditAttribution: burgerboydaddy commentedComment #21
burgerboydaddy CreditAttribution: burgerboydaddy commentedResults of manual comparison:
Original - clean
Patched
Comment #22
burgerboydaddy CreditAttribution: burgerboydaddy commentedComment #23
burgerboydaddy CreditAttribution: burgerboydaddy commentedResults of manual comparison:
Original - clean
Patched
Comment #24
joelpittetGreat work all! This is ready to go. Patch passes testbot and Profiling and Manual Testing complete.
@burgerboydaddy thanks for the profiling and manual reviews! And joining in on the sprint in Vancouver.
Comment #25
star-szrThanks everyone!
Do we still need this @todo? It doesn't seem accurate anymore.
Comment #26
joelpittetWe do not need that in there, That issue can deal with itself when we get to them, I'd rather not commit code with @todo's if we can help it. Please remove:)
Comment #27
star-szrContributor task up for grabs, then!
Remove the two offending lines from the Twig template and post a new patch and interdiff :)
Comment #28
InternetDevels CreditAttribution: InternetDevels commentedRemoved deprecated lines and added interdiff :)
Comment #30
InternetDevels CreditAttribution: InternetDevels commented28: convert-theme-textarea-2152229-28.patch queued for re-testing.
Comment #31
snap_xTest passed. Needs review.
Comment #32
joelpittetThanks @InternetDevels for removing those todo lines.
This has been profiled in #23 and markup compare in #25. Back to RTBC:)
Comment #33
star-szrAdding some folks to the proposed commit message :)
Comment #34
webchickCommitted and pushed to 8.x. Thanks!