Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
On my first pass these seem relatively related to be accommodated by a single patch. If it gets unwieldily, I'll create separate issues.
Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render()
Comment | File | Size | Author |
---|---|---|---|
#65 | interdiff-60-65.txt | 8.44 KB | martin107 |
#65 | replace-theme-in-includes-2007124-65.patch | 1.46 KB | martin107 |
#60 | replace-theme-in-includes-2007124-60.patch | 9.9 KB | InternetDevels |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commented@c4rl, feel free to re-assign but somebody else will probably pick this up for you :)
Comment #2
anirudha_3083 CreditAttribution: anirudha_3083 commentedComment #3
anirudha_3083 CreditAttribution: anirudha_3083 commentedcovered authorize.php, install.php, update.php and core/includes/* files. Let's try
Comment #5
anirudha_3083 CreditAttribution: anirudha_3083 commentedLooks like "theme_form_element_label" receives $variables as parameter. Cannot change theme() to drupal_render() in form.inc at lines 4731 & 4737
Comment #6
anirudha_3083 CreditAttribution: anirudha_3083 commentedforgot to change the status.
Comment #8
anirudha_3083 CreditAttribution: anirudha_3083 commentedmessed up with previous patch due to old master repository. Created a fresh patch with latest repository but #5 still pending.Feel free to assign it to yourself and complete this.
Comment #10
anirudha_3083 CreditAttribution: anirudha_3083 commentedFixed the issues in last patch. Re-uploading the patch. #5 still remains.
Comment #11
anirudha_3083 CreditAttribution: anirudha_3083 commentedComment #13
anirudha_3083 CreditAttribution: anirudha_3083 commentedincorrect variable name was passed. Fixed. Re-submitting the patch.
Comment #15
star-szrUnassigning, it's been a while. Anyone is welcome to work on this.
Comment #16
pplantinga CreditAttribution: pplantinga commentedFeel free to re-assign if you want this again anirudha
Here's my stab at it. I wasn't about to sort through all those fails/exceptions, so here's a clean slate attempt.
Comment #18
pplantinga CreditAttribution: pplantinga commentedRunning into the same problem as anirudha: can't convert theme() to drupal_render() in form.inc at lines 4731 & 4737 because theme_form_element_label takes a render element. We've run into this issue on several other occasions, e.g. #2041283: theme_status_report() is severely broken
One solution is to change the argument from a 'render element' to a 'variable', as in the referenced issue. Another is to fix theme_form_element_label.
Comment #20
pplantinga CreditAttribution: pplantinga commentedApparently I can't spell maintenance. "mantenance" and "mainenance" both happened.
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedIf we need to open a new issue for this to be cleaned up, I think we should do so.
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commented#2046849: Fix theme_form_element_label so that it is compatible with drupal_render()
Comment #23
pplantinga CreditAttribution: pplantinga commentedRe-roll.
Comment #24
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedupdate.php was done in this issue: #2009688: Replace theme() with drupal_render() in update system.
Comment #25
pplantinga CreditAttribution: pplantinga commentedThis implements the change in #2046849: Fix theme_form_element_label so that it is compatible with drupal_render()
Let's see if it passes tests.
Comment #26
pplantinga CreditAttribution: pplantinga commentedCool. Let's get #2046849: Fix theme_form_element_label so that it is compatible with drupal_render() in.
Comment #27
Eric_A CreditAttribution: Eric_A commentedThis is why the tests were failing.
See my comments in #2030805: Make theme_status_report compatible with drupal_render() for the correct way to build the render element for this type of theme hook and then you won't need to change the type of the theme_hook.
Comment #28
pplantinga CreditAttribution: pplantinga commentedOkay, here's the updated patch.
Comment #30
Eric_A CreditAttribution: Eric_A commented#28: replace_theme_in_includes-2007124-28.patch queued for re-testing.
Comment #32
Eric_A CreditAttribution: Eric_A commentedThe $form_required_marker
element needs the same fix as $form_element_label...Here I think it is better to be consistent and use $element instead of $variables['element'].
Also, do we need the new line before building the $form_element_label element?
Comment #33
thedavidmeister CreditAttribution: thedavidmeister commentedrelated #2060401: Remove useless "early render" in authorize.php
Comment #34
pplantinga CreditAttribution: pplantinga commentedHere are changes @Eric_A recommended, but manual testing shows that it isn't going to work. #2046849: Fix theme_form_element_label so that it is compatible with drupal_render() needs to happen first.
Comment #36
pplantinga CreditAttribution: pplantinga commentedStuck on #2046849: Fix theme_form_element_label so that it is compatible with drupal_render()
Comment #37
pplantinga CreditAttribution: pplantinga commentedHow about this? Just put only the things that theme_form_element_label needed into the array. Checked them with the same method in #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter().
At some point $element should stop being sent to theme_form_required_marker, since it doesn't actually need it. Either that or it should handle the required logic.
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commentedSounds like a followup issue?
Comment #39
pplantinga CreditAttribution: pplantinga commentedFollow up issue: #2062477: theme_form_required_marker does not use the render element it receives as a parameter
Comment #41
pplantinga CreditAttribution: pplantinga commentedWell at least it installed...
It looks like to fix it the rest of the way we'll have to include the fix in the follow-up issue. So this either needs to be postponed, or that issue should just be marked as duplicate or whatever.
Also, since we're only passing part of a render element, it's starting to look a whole lot like variables, not render elements. I think passing render elements to multiple theme() functions can't be changed to drupal_render() because it double-renders them...
Comment #42
pplantinga CreditAttribution: pplantinga commentedComment #44
Eric_A CreditAttribution: Eric_A commentedDon't have much time now to look into this, but perhaps this is an issue with us passing a new version of an element (copy with changed #theme property) instead of passing the same variable or a passing a reference to the variable. This means we should take care to proceed with the correct variable where it matters. (Or stop starting out with a copy, but just change the theme property.)
Might this be the code that creates the double rendering?
Comment #45
Eric_A CreditAttribution: Eric_A commentedYeah, we need the #children property of $form_element_label here, not of $element. @pplantinga, can you try the effect on the patch from #34?
Comment #46
Eric_A CreditAttribution: Eric_A commented#45 was shouted out in a hurry, lets just think this through a bit. I don't want to rush anyone into making changes that are not analyzed well.
Comment #47
pplantinga CreditAttribution: pplantinga commentedWell here's one option that (hopefully) passes tests. I guess I'll give Eric_A's suggestion a try.
Comment #49
pplantinga CreditAttribution: pplantinga commented#47: replace_theme_in_includes-2007124-47.patch queued for re-testing.
Comment #50
pplantinga CreditAttribution: pplantinga commentedWas not able to make Eric_A's method work. Perhaps he should take a crack at it if this isn't good enough.
Comment #51
drupalmonkey CreditAttribution: drupalmonkey commentedIs this issue actually ready to be reviewed? Is this issue still waiting on #2046849: Fix theme_form_element_label so that it is compatible with drupal_render()??
Also, if it is ready for review, how to test the patch for this?
Comment #52
royko_at_duo CreditAttribution: royko_at_duo commentedPatch #47 works for me...no instances of theme() found in any of these files after installing patch. Reviewed relevant affected pages.
Notes:
-- #41 Double rendering issue being addressed in drupal.org/node/1920886
Comment #53
Eric_A CreditAttribution: Eric_A commented@pplantinga, I can see that you tried to implement my remark in #32 about
theme_form_required_marker()
. I think you got confused by what is in fact very confusing indeed: this function should be passed a render element (in the end mapped to a theme hook variable key "element") which has a *child* element named "element". Core doesn't use this child element in its implementation oftheme_form_required_marker()
and it seems there are no tests for this in test themes.Comment #54
jenlamptonRelated: #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead
Comment #55
pplantinga CreditAttribution: pplantinga commentedIf #47 isn't good enough, then this will have to wait for #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead
Comment #56
markhalliwell47: replace_theme_in_includes-2007124-47.patch queued for re-testing.
Comment #58
markhalliwellComment #59
star-szrMost of this territory is now covered in new sub-issues of the parent issue, many of which are in needs review. So leaving this open in case there are some strays but a reroll is probably not going to be productive.
Comment #60
InternetDevels CreditAttribution: InternetDevels commentedPatch added.
Comment #62
me-taras CreditAttribution: me-taras commented60: replace-theme-in-includes-2007124-60.patch queued for re-testing.
Comment #64
star-szrAlright, I looked at the other issues in the parent meta and authorize.php and install.php are not covered, but I couldn't find any occurrences in install.php.
So the patch here should only change authorize.php.
Comment #65
martin107 CreditAttribution: martin107 commentedRemoved all changes except those in core/authorize.php
Messages in test output are not enlightening ... will look again when bot comes back
Comment #67
martin107 CreditAttribution: martin107 commentedThe failure point in the test is highlighted below
It is not related to the patch. So HEAD should also fail here.
Also running this local work with and without the patch
show that everything passes... asking for retest
Comment #68
martin107 CreditAttribution: martin107 commented65: replace-theme-in-includes-2007124-65.patch queued for re-testing.
Comment #69
martin107 CreditAttribution: martin107 commentedComment #70
pplantinga CreditAttribution: pplantinga commentedLooks good to me! pretty straightforward
Comment #71
catchCommitted/pushed to 8.x, thanks!