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

CommentFileSizeAuthor
#7 2190427-form-render-element-breaking.patch701 bytesjoelpittet
#5 2108771-special-attribute-keys-40-reroll.patch9.11 KBjoelpittet
#3 form-inc-theme-func-2190427-3.patch1.18 KBjessebeach
form-inc-theme-func.patch1.13 KBjessebeach

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Related issues: +#2152215: Convert theme_form_element_label() to Twig

Thanks!

+++ b/core/includes/form.inc
@@ -2851,7 +2851,14 @@ function theme_form_element_label($variables) {
+    $marker = array(
+      '#theme' => 'form_required_marker',
+      '#element' => $element
+    );

Should have a comma after $element.

Status: Needs review » Needs work

The last submitted patch, form-inc-theme-func.patch, failed testing.

jessebeach’s picture

jessebeach commented
Status: Needs work » Needs review
StatusFileSize
new form-inc-theme-func-2190427-3.patch1.18 KB

Comma added.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

#2152215: Convert theme_form_element_label() to Twig

This is also in the twig conversion. But I'm concerned about that #element => $element there too because of the weird way that 'render element' treats the incoming variables through drupal_render(). In this case form_required_marker doesn't use any incoming variables so it should pass tests no problem. A quick test that that the $variables['element'] has the right things on that required marker. If I get a chance I'll do that, otherwise someone may want to jump on that.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
StatusFileSize
new 2108771-special-attribute-keys-40-reroll.patch9.11 KB

Demo of test breaks and what I mean.

Status: Needs review » Needs work

The last submitted patch, 5: 2108771-special-attribute-keys-40-reroll.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new 2190427-form-render-element-breaking.patch701 bytes

Whoops

jessebeach’s picture

jessebeach commented

It's true, the $element passed as #element is not needed for theme_form_required_marker. We could just as easily pass an empty array since the $variables argument to the theme function is never referenced in the theme function.

I would rather deal with that weirdness in #2152215: Convert theme_form_element_label() to Twig, where the mandate is to untangle these inconsistencies and introduce a TWIG template. This issue here is simply concerned with eliminating calls to theme(), which we can safely do with the patch in #3.

Status: Needs review » Needs work

The last submitted patch, 7: 2190427-form-render-element-breaking.patch, failed testing.

martin107’s picture

martin107 commented

+1 on #8 -- untangle inconsistencies has it place ... in another issue.

joelpittet’s picture

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

7: 2190427-form-render-element-breaking.patch queued for re-testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

The problem has been made aware to all involved and will be dealt with in the twig conversion. I'm RTBCing this as to not hold this up:)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2190427-form-render-element-breaking.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Reviewed & tested by the community

Sorry messed this up... RTBC #3

star-szr’s picture

star-szr
he/him
Primary language English
commented

Hiding everything but #3 :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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