Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().

CommentFileSizeAuthor
#14 2177653-form-drupal-render-14.patch1.3 KBjoelpittet
#8 interdiff.txt949 bytesjessebeach
#8 form-drupal-render-2177653-8.patch3 KBjessebeach
#5 interdiff.txt3.64 KBjessebeach
#5 replace-theme-drupal-render-form-inc-2177653-5.patch3.01 KBjessebeach
#5 labels-2.png157.58 KBjessebeach
#1 replace-theme-with-drupal-render-2177653.patch2.72 KBinternetdevels

Comments

internetdevels’s picture

internetdevels commented
Status: Active » Needs review
StatusFileSize
new replace-theme-with-drupal-render-2177653.patch2.72 KB

Patch added.

Status: Needs review » Needs work

The last submitted patch, 1: replace-theme-with-drupal-render-2177653.patch, failed testing.

internetdevels’s picture

internetdevels commented
Status: Needs work » Needs review

1: replace-theme-with-drupal-render-2177653.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: replace-theme-with-drupal-render-2177653.patch, failed testing.

jessebeach’s picture

jessebeach commented
StatusFileSize
new labels-2.png157.58 KB
new replace-theme-drupal-render-form-inc-2177653-5.patch3.01 KB
new interdiff.txt3.64 KB

I 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_library with #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_test module and navigating to this path: form_test/tableselect/multiple-true.

jessebeach’s picture

jessebeach commented
Status: Needs work » Needs review

go bot, go.

Status: Needs review » Needs work

The last submitted patch, 5: replace-theme-drupal-render-form-inc-2177653-5.patch, failed testing.

jessebeach’s picture

jessebeach commented
Status: Needs work » Needs review
StatusFileSize
new form-drupal-render-2177653-8.patch3 KB
new interdiff.txt949 bytes

Test error because empty('0') was evaluating to false. Switched to isset() instead.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Patch seems fine to me RTBC for me but let's see what twig guys think about it.

moshe weitzman’s picture

moshe weitzman commented

The 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'

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

This 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)

jessebeach’s picture

jessebeach commented

Great 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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Committed that one.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new 2177653-form-drupal-render-14.patch1.3 KB

Re-rolled #8 without #2152215: Convert theme_form_element_label() to Twig too.

Status: Needs review » Needs work

The last submitted patch, 14: 2177653-form-drupal-render-14.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review

14: 2177653-form-drupal-render-14.patch queued for re-testing.

martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community

clean implementation +1

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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