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.| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2177653-form-drupal-render-14.patch | 1.3 KB | joelpittet |
| #8 | interdiff.txt | 949 bytes | jessebeach |
| #8 | form-drupal-render-2177653-8.patch | 3 KB | jessebeach |
| #5 | interdiff.txt | 3.64 KB | jessebeach |
| #5 | replace-theme-drupal-render-form-inc-2177653-5.patch | 3.01 KB | jessebeach |
Comments
Comment #1
InternetDevels CreditAttribution: InternetDevels commentedPatch added.
Comment #3
InternetDevels CreditAttribution: InternetDevels commented1: replace-theme-with-drupal-render-2177653.patch queued for re-testing.
Comment #5
jessebeach CreditAttribution: jessebeach commentedI have just a few tweaks for this one. I ran into trouble with the form label changes. Labels were not rendering:
The form item label changes can be verified on an article add/edit page. Look for labels on the fields and the required marker on the title field.
For the tableselect theme function, we have the opportunity to replace
drupal_add_librarywith#attached, which gets us further along resolving #2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses.The tableselect theme changes can be verified by enabling the
form_testmodule and navigating to this path:form_test/tableselect/multiple-true.Comment #6
jessebeach CreditAttribution: jessebeach commentedgo bot, go.
Comment #8
jessebeach CreditAttribution: jessebeach commentedTest error because
empty('0')was evaluating to false. Switched toisset()instead.Comment #9
jibranPatch seems fine to me RTBC for me but let's see what twig guys think about it.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedThe goal is not to simply replace remove calls to theme() with drupal_render(). It is a code smell to call drupal_render() in regular code. That should just be called by "the framework". When we are within a theme() call, we should not need to do internal (i.e. nested) themeing.
In this patch, can we just kill
theme('form_element_label')or integrate it into another theme() hook?As for theme_tableselect, isn't that deprecated now in favor of a #type = 'table' and #tableselect = TRUE. See https://drupal.org/node/1876710. Might be worth deleting it and seeing what breaks. If something breaks, it can be fixed as part of #1876712: [meta] Convert all tables in core to new #type 'table'
Comment #11
joelpittetThis patch does very similar things #2152213: Convert theme_form_element() to Twig and the twig conversion. Will cause a re-roll for which gets in first.
And here's another plan for table select:
#2152227: Convert theme_tableselect() to #theme table__tableselect (see not implemented)
Comment #12
jessebeach CreditAttribution: jessebeach commentedGreat joelpittet! I'd rather see #2152213: Convert theme_form_element() to Twig get committed first. It's the best solution here. I reviewed your patch and it looks good - RTBC. Once it gets in we're then only left with the table issue here.
Comment #13
catchCommitted that one.
Comment #14
joelpittetRe-rolled #8 without #2152215: Convert theme_form_element_label() to Twig too.
Comment #16
joelpittet14: 2177653-form-drupal-render-14.patch queued for re-testing.
Comment #17
martin107 CreditAttribution: martin107 commentedclean implementation +1
Comment #18
catchCommitted/pushed to 8.x, thanks!