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.
This is the part where the test is failing. If I leave the $status as using the theme() function instead of building the array for the render, all the tests pass. Any idea why?
@tsphethean I've worked further on this trying to figure out why i'm not able to switch the theme function as mentioned in #3 but no luck. All tests pass if you undo the changes to that status_report theme.
Hope you have better luck, and please if you figure it out do let me know what I missed.
I'm getting the same SimpleTest errors in the Update Functionality tests regardless of whether I have this patch applied or not so want to see what testbot thinks, just in case its something in my setup.
Ok, tests were failing for other instances of theme('status_report') so have updated them too, and then tweaked theme_status_report() to accept the new variables passed in from drupal_render().
Just did a quick test, and if I update the theme function without doing the drupal_render conversion then it will fail... so should the new issue also do the conversions for all places that theme is used?
@tspphethean - yes, if the new issue was "make theme_status_report compatible with drupal_render()" and that was only achievable by making it incompatible with theme() then you would need to perform the conversion there - but that's exactly the kind of discussion we would be having under the new issue.
This still leaves us with theme('status_report') to be converted in install.core.inc and SystemInfoController::status() - but I think they are covered by other issues.
Tor Arne ThuneCreditAttribution: Tor Arne Thune commented
Title:
Replace theme() with drupal_render() in update module
» Replace theme() with drupal_render() in update system
Changing title so that it's not confused with the update module in Drupal core. The patches in this issue touch the update system, not the update (manager) module.
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedwrong module...
Comment #2
aaronott CreditAttribution: aaronott commentedComment #3
aaronott CreditAttribution: aaronott commentedSo I'm having an issue with this section
This is the part where the test is failing. If I leave the $status as using the theme() function instead of building the array for the render, all the tests pass. Any idea why?
Comment #4
Samvel CreditAttribution: Samvel commentedthis code part looks good
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commentedfeel free to re-assign if you're still working on this @aaronott
Comment #7
tsphethean CreditAttribution: tsphethean commentedComment #8
aaronott CreditAttribution: aaronott commented@tsphethean I've worked further on this trying to figure out why i'm not able to switch the theme function as mentioned in #3 but no luck. All tests pass if you undo the changes to that status_report theme.
Hope you have better luck, and please if you figure it out do let me know what I missed.
Comment #9
tsphethean CreditAttribution: tsphethean commented#3: 2009688-replace_theme_with_drupal_render_in_update-3.patch queued for re-testing.
I'm getting the same SimpleTest errors in the Update Functionality tests regardless of whether I have this patch applied or not so want to see what testbot thinks, just in case its something in my setup.
Comment #11
tsphethean CreditAttribution: tsphethean commentedTests were failing because it looks like the structure of $requirements has changed, hopefully this should do it.
Comment #13
tsphethean CreditAttribution: tsphethean commentedOk, tests were failing for other instances of theme('status_report') so have updated them too, and then tweaked theme_status_report() to accept the new variables passed in from drupal_render().
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedif we're changing the way that a theme function works, we need to open a new issue for that.
Comment #15
tsphethean CreditAttribution: tsphethean commentedJust did a quick test, and if I update the theme function without doing the drupal_render conversion then it will fail... so should the new issue also do the conversions for all places that theme is used?
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commented@tspphethean - yes, if the new issue was "make theme_status_report compatible with drupal_render()" and that was only achievable by making it incompatible with theme() then you would need to perform the conversion there - but that's exactly the kind of discussion we would be having under the new issue.
When you open that issue, could you link to it from #2015147: [meta] Improve the DX of drupal_render(), renderable arrays and the Render API in general as well?
Comment #17
tsphethean CreditAttribution: tsphethean commented@thedavidmeister - no problem, thanks for the help.
Opened #2030805: Make theme_status_report compatible with drupal_render() and added it to #2015147: [meta] Improve the DX of drupal_render(), renderable arrays and the Render API in general
Comment #18
heddnNeed to resolve #2030805: Make theme_status_report compatible with drupal_render() first.
Comment #19
Eric_A CreditAttribution: Eric_A commented#2030805: Make theme_status_report compatible with drupal_render() has been resolved.
Comment #20
Eric_A CreditAttribution: Eric_A commented#2030805: Make theme_status_report compatible with drupal_render() is partially resolved. Also, another approach is up for discussion in #2041283: theme_status_report() is severely broken.
Comment #21
star-szrRe-activating since #2041283: theme_status_report() is severely broken was committed. I haven't read through this thoroughly but I'm guessing the patch will need updating.
Comment #22
tsphethean CreditAttribution: tsphethean commentedOk, so #2041283: theme_status_report() is severely broken has gone in, which means this can go back to pretty much the same patch as #3 and should now pass (seems good locally).
This still leaves us with theme('status_report') to be converted in install.core.inc and SystemInfoController::status() - but I think they are covered by other issues.
Comment #23
pplantinga CreditAttribution: pplantinga commentedOne small coding style nitpick: two arrays are missing commas after the last item
Comment #24
tsphethean CreditAttribution: tsphethean commentedMissing commas added - thanks @pplantinga
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commentedLooks good to me.
Comment #26
alexpottCommitted 36d5732 and pushed to 8.x. Thanks!
Comment #27
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedChanging title so that it's not confused with the update module in Drupal core. The patches in this issue touch the update system, not the update (manager) module.
Comment #28
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedCreated an issue for the update module: #2048933: Replace theme() with drupal_render() in update module.