Problem/Motivation
Usage of the theme() function directly invokes the theme layer and markup rendering directly prior to other calls and alterations that may be necessary to adjust the resulting markup sent to the user agent.
Moving to render arrays from D6 to D7 provided an abstraction of how markup would eventually be constructed so that it could be extended and altered throughout page execution before finally invoking a theme callback. In order for full compliance, any usage of theme() returning markup should instead return renderable arrays.
Much of this conversion didn't fully take place in during development from D6 to D7. Given our effort to refactor how Render API operates, we need to start with a comprehensive usage of drupal_render().
Proposed resolution
Only drupal_render() should invoke theme(). Convert all calls to theme() to renderable arrays which are then rendered via drupal_render().
// Old:
$output = theme('foo', array('bar' => $bar));
return $output;
// New:
$foo = array(
'#theme' => 'foo',
'#bar' => $bar,
);
return drupal_render($foo);
Reviewer's/writer's notes
Do NOT attempt to do this (you'll get a fatal error when drupal_render() tries to modify your array by reference):
$output = drupal_render(array('#theme' => 'foo', '#bar' => $bar));
Always create the renderable array and then render it in two steps.
If presented with something like this:
function theme_image_crop_summary($variables) {
return theme('image_resize_summary', $variables);
}
then be explicit with the renderable array and do this:
function theme_image_crop_summary($variables) {
// #theme image_resize_summary expects 'foo' and 'bar'.
$image_resize_summary = array(
'#theme' => 'image_resize_summary',
'#foo' => $variables['foo'],
'#bar' => $variables['bar'],
);
return drupal_render($image_resize_summary);
}
do NOT do something like this:
foreach($variables as $key => $variable) {
$variables["#$key"] = $variable;
}
Other general guidelines
- In Drupal 8 the l() function calls drupal_render() on the first parameter passed to it if that parameter is an array. This means
$foo = ('#markup' => 'bar'); l(drupal_render($foo), $path, $options);
is superfluous, the drupal_render() inline with l() should be avoided here, ie. usel($foo, $path, $options);
in this case. - The name of the variable for the renderable array should be the same as '#theme', (ie. $foo = array('#theme' => 'foo', ... );) if/when this would conflict with an existing variable in the current scope, $build is the alternative.
- Don't try to do anything new/fancy like returning the renderable array instead of rendering it where theme() was, despite this probably being "better" in many cases it will also incur quite a bit of extra testing overhead. We're trying to minimise the scope of this issue. Don't change the name/structure of any existing variables or try to add/remove/merge any existing functions. Touch nothing but the lamp (@see Aladdin). If the patch does change anything else, it will need manual testing and therefore slow/stall what should be relatively simple tasks. @thedavidmeister, who wrote this rule, is aware of the irony/hypocrisy in that the patches he submitted (before writing this rule) all refactored things slightly when they probably shouldn't have :P
- If the conversion is a simple old -> new as above with no other refactor, a careful review should be enough for RTBC. Anything more will require manual testing. Anything sitting behind AJAX, like the views UI requires manual testing too as the testbots can't see it.
- There is a Drupal.theme function being called in JavaScript files but this is not related to this conversion.
Remaining tasks
Fixed, yay!
- seven - #2006952: Replace theme() with drupal_render() in seven.theme
- action - #2008968: Replace theme() with drupal_render() in action module
- edit - #2009002: Replace theme() with drupal_render() in edit module
- tracker - #2009686: Replace theme() with drupal_render() in tracker module
- node - #2009660: Replace theme() with drupal_render() in node module
- views - #2007052: Replace theme() with drupal_render() in views.module
- language - #2009652: Replace theme() with drupal_render() in language module
- ckeditor - #2008976: Replace theme() with drupal_render() in ckeditor module
- tour - #2009684: Replace theme() with drupal_render() in tour module
- book - #2008974: Replace theme() with drupal_render() in book module
- datetime - #2008986: Replace theme() with drupal_render() in datetime module
- menu - #2009658: Replace theme() with drupal_render() in menu module
- core/lib - #2008954: Replace theme() with drupal_render() in /core/lib
- search - #2009666: Replace theme() with drupal_render() in search module
- shortcut - #2009668: Replace theme() with drupal_render() in shortcut module
- history - #2009578: Replace theme() with drupal_render() in history module
- rdf - #2009664: Replace theme() with drupal_render() in rdf module
- user - #2009690: Replace theme() with drupal_render() in user module
- dblog - #2008990: Replace theme() with drupal_render() in dblog module
- comment - #2008980: Replace theme() with drupal_render() in comment module
- theme_image_crop_summary - #2010122: Replace theme() with drupal_render() in image module for theme_image_crop_summary()
- views UI - #2006974: Replace theme() with drupal_render() in views_ui.module
- theme_image_style #2010134: Replace theme() with drupal_render() in image module for theme_image_style()
- config - #2008982: Replace theme() with drupal_render() in config module
- theme_system_themes_page - #2010086: Replace theme() with drupal_render() in system module for theme_system_themes_page()
- taxonomy - #2009680: Replace theme() with drupal_render() in taxonomy module
- update system - #2009688: Replace theme() with drupal_render() in update system
- field & field_ui - #2009008: Replace theme() with drupal_render() in field and field_ui modules
- filter - #2009018: Replace theme() with drupal_render() in filter module
- file - #2009014: Replace theme() with drupal_render() in file module
- theme_image_formatter - #2010126: Replace theme() with drupal_render() in image module for theme_image_formatter()
- editor - #2053731: Replace theme() with drupal_render() in editor
- forum - #2009574: Replace theme() with drupal_render() in forum module Assigned to: e2tha-e
- aggregator - #2008970: Replace theme() with drupal_render() in aggregator module
- update - #2048933: Replace theme() with drupal_render() in update module
- locale - #2009654: Replace theme() with drupal_render() in locale module
- image - #2009580: Replace theme() with drupal_render() in image module
- simpletest - #2009670: Replace theme() with drupal_render() in simpletest module
- system - #2009674: Replace theme() with drupal_render() in system module
- batch.inc - #2177643: Replace theme() with drupal_render() in batch.inc
- locale.bulk.inc - #2177667: Replace theme() with drupal_render() in locale.bulk.inc
- SearchExtraTypeSearch - #2177673: Replace theme() with drupal_render() in Drupal/search_extra_type/Plugin/Search/SearchExtraTypeSearch.php
- ajax.inc - #2177637: Replace theme() with drupal_render() in ajax.inc
- common.inc - #2177645: Replace theme() with drupal_render() in common.inc
- errors.inc - #2177651: Replace theme() with drupal_render() in errors.inc
- theme.maintenance.inc - #2177665: Replace theme() with drupal_render() in theme.maintenance.inc
- theme_form_element to TWIG - #2152213: Convert theme_form_element() to Twig
- form.inc - #2177653: Replace theme() with drupal_render() in form.inc
- tablesort.inc - #2177657: Replace theme() with drupal_render() in tablesort.inc
- theme.inc - #2177663: Replace theme() with drupal_render() in theme.inc
- core/includes - #2007124: Replace theme() with drupal_render() in authorize.php
- install.core.inc - #2177655: Replace theme() with drupal_render() in install.core.inc
- includes/form.inc = #2190427: Replace theme() with drupal_render() in form.inc
Good to go
- EMPTY!!!
Has patch, needs review
- EMPTY!!!
Needs patch/further work
- EMPTY!!!
Postponed temporarily
Calls to theme()
https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/c...
User interface changes
None :)
API changes
None :)
Related Issues
- #1843798: [meta] Refactor Render API to be OO
- #2031319: The documentation for theme() should be clearer about saying not to call it directly.
- #2015147: [meta] Improve the DX of drupal_render(), renderable arrays and the Render API in general
- #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead
- This is a blocking issue for #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system
Original report by @thedavidmeister
There's a bunch of discussion around what we can do to make rendering and the theme system better. Currently there are ideas blocked simply because sometimes theme() is called inside drupal_render() and sometimes it isn't. [...] We should just cleanup what direct theme() calls are left in core so it doesn't hold back refactors elsewhere
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedThis should be fun. :)
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedyeah... not sure how we want to organise this one. I guess just dive in and see where we can find patterns in the chaos?
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedi'm going in..
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedI think by module/theme is the way to go. I'll start adding remaining tasks as I roll patches.
Comment #5
c4rl CreditAttribution: c4rl commentedI'll also add some tasks as well.
Comment #6
andypostthere could be a lot of novice issues for sprint
Comment #6.0
andypostDenote that the remaining task list isn't comprehensive.
Comment #7
stevectorI added a link to https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/c... which has the number of calls to theme() as 192.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commented#7 - you're right, the grep for theme(' provides an overestimate, while I've been looking through core I've seen that it matches documentation, some javascript files and some debug code as well as actual theme() calls.
Comment #8.0
thedavidmeister CreditAttribution: thedavidmeister commentedAdding a link to api.drupal.org
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedok, issues are up. I think I've found them all but I'll definitely give it all another sweep once we get through these.
Comment #10
Samvel CreditAttribution: Samvel commentedWhat to do, if we have this situation?
Comment #11
andypost@Samvel suppose better remove useless functions so introduce one
theme_image_effect_summary()
and convert similar calls to new functionComment #11.1
star-szrSimplify conversion instructions after IRC discussion
Comment #11.11
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commented#11 - I'm not sure I agree with that. We're not looking to add/remove any functions here, just get things routing through drupal_render() and staying functionally equivalent otherwise.
I don't know if this is the right thing either but I'd say have a look at what theme_image_resize_summary needs and convert based on that:
say $variables looks like this:
and theme_image_resize_summary expects $variables('foo' = 'somedefault', 'bar' = 'anotherdefault').
Then do this, I suppose:
Comment #13
andypostI'd prefere
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedor just?
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commented#13 is probably better than #14 and #12 as it forces us to be explicit and is more efficient.
Comment #16
InternetDevels CreditAttribution: InternetDevels commentedWe are working today with this issue during Code Sprint UA.
Comment #17
Samvel CreditAttribution: Samvel commentedThank you to both. Clear for me.
Comment #18
Samvel CreditAttribution: Samvel commented@InternetDevels, sorry, not specially.
Comment #18.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #18.12
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #18.22
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #18.26
star-szrRemove block issue from 'fixed' section - no theme() calls there.
Comment #18.30
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #18.32
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedI've noticed this in a few patches waiting for review:
In d8 literally the first thing l() does is calls drupal_render() on the $text parameter if it is an array, so it's a waste to call drupal_render() for l() like this.
Comment #20
tstoecklerHow would that be done correctly, though? That's not clear to me.
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedbecomes:
Comment #22
sbudker1 CreditAttribution: sbudker1 commentedBefore code:
After code:
Patch seemed to work! The visuals of the cite looked the same after the patch. In addition the code also looks the same!
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commented#22 - looks like a cross post?
Comment #23.0
thedavidmeister CreditAttribution: thedavidmeister commentedmoved history
Comment #23.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #23.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #23.3
jenlamptoncleanup
Comment #23.4
jenlamptonmoar cleanup
Comment #23.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #23.6
jenlamptoncleanup
Comment #23.7
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #23.8
jenlampton.
Comment #23.9
jenlamptonreorg
Comment #23.10
jenlamptonreorg
Comment #23.11
jenlamptonreorg
Comment #23.12
jenlamptonreorg
Comment #23.13
jenlamptonforum
Comment #24
jenlamptonyes, #22 was a cross post. sorry about that :)
Comment #24.0
jenlamptontax needs work
Comment #24.1
jenlamptonfor colors
Comment #24.2
jenlamptonreorder
Comment #24.3
jenlamptonunpostpone
Comment #24.4
jenlamptonspacing
Comment #24.5
jenlamptonup
Comment #24.6
jenlamptonreorg
Comment #24.7
jenlamptonreorg
Comment #24.8
jenlamptonreorg
Comment #24.9
jenlamptonupdate
Comment #24.10
jenlamptonreorg
Comment #24.11
jenlamptonreorg
Comment #24.12
heddnmoved to completed:
Comment #24.13
heddnUpdated issue summary.
Comment #24.14
heddnUpdated issue summary.
Comment #24.15
heddnUpdated issue summary.
Comment #24.16
heddnUpdated issue summary.
Comment #24.17
heddnUpdated issue summary.
Comment #24.18
heddnUpdated issue summary.
Comment #24.19
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #24.20
jenlamptonreorg
Comment #24.21
jenlamptonreorg
Comment #24.22
jenlamptonreorg
Comment #24.23
star-szrMove up taxonomy
Comment #24.24
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #24.25
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #24.26
jenlamptonreorg
Comment #24.27
jenlamptonreorg
Comment #24.28
jenlamptonreorg
Comment #24.29
jenlamptonreorg
Comment #24.30
jenlamptonup
Comment #24.31
star-szrReshuffle
Comment #24.32
jenlamptonup
Comment #25
Eric_A CreditAttribution: Eric_A commentedI've noticed conversions of low level tests around theme() and theme hook implementation output that should not be converted. These tests are a different kind of animal and need a different kind of review. Splitting off test files would speed up these issues, especially for the issues where conversion of the normal files is considered a "Novice" task. See for example #2009670: Replace theme() with drupal_render() in simpletest module and #2009674: Replace theme() with drupal_render() in system module.
Comment #26
thedavidmeister CreditAttribution: thedavidmeister commented@Eric_A, could you update the issue summary here and reorganise the remaining sub-issues as required as per #25?
Comment #26.0
thedavidmeister CreditAttribution: thedavidmeister commentedup
Comment #26.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #26.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #27
thedavidmeister CreditAttribution: thedavidmeister commentedRelated #2031319: The documentation for theme() should be clearer about saying not to call it directly.
Comment #27.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #27.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #27.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #27.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #27.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #27.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28
thedavidmeister CreditAttribution: thedavidmeister commentedRelated #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead
Comment #28.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.1
jenlamptonup
Comment #28.2
jenlamptonup
Comment #28.3
stevectorFixing spelling of "temporarily"
Comment #28.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.6
jenlamptonseeing if summary needs update
Comment #28.7
jenlamptonnr
Comment #28.8
jenlamptonup
Comment #28.9
jenlamptonup
Comment #28.10
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedCleared up a misunderstanding around the update system and the update module, as they are two different things.
Comment #28.11
jenlamptonup
Comment #28.12
jenlamptonup
Comment #28.13
jenlamptonup
Comment #28.14
jenlamptonup
Comment #28.15
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.16
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.17
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.18
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.19
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.20
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.21
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.22
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.23
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.24
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.25
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.26
jenlamptonup
Comment #28.27
jenlamptonreorder
Comment #28.28
jenlamptonp
Comment #28.29
jenlamptonf
Comment #28.30
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.31
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.32
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.33
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.34
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.35
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.36
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #28.37
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #29
thedavidmeister CreditAttribution: thedavidmeister commentedGuys, just a quick reminder that you do *not* have to try too hard to remove "early render" calls to drupal_render() in these issues. If you get some easy/obvious ones, great, otherwise don't stress it too much.
There is already a follow-up meta that is set to explode with tasks in the near future over at #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead so don't feel like the cleanup will just disappear into the ether if we don't get it immediately here.
We're really on the home stretch now (nearly on the last 50 calls), so if there's a choice between doing a quick conversion "1:1" or trying harder for "further cleanup", let's opt for the former and just get this out the door :)
I'm going to do my best to keep an eye on regressions coming in from other other patches around core that have been introducing new theme() calls that didn't exist when we did our first audit.
Comment #30
thedavidmeister CreditAttribution: thedavidmeister commentedOh, and much like the Twig conversion efforts, what we need most right now are reviews.
Right now it seems like quality feedback on patches is much harder to come by than the patches themselves.
If you can spare half an hour to look over one of the yellow issues in the summary for coding standards violations or patches that don't follow the guidelines outlined in the issue summary, it would be super helpful to getting this wrapped up.
Comment #30.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.1
jenlamptonup
Comment #30.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.4
star-szrUpdate
Comment #30.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.6
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.7
jenlamptonupdate
Comment #30.8
jenlamptonupdate
Comment #30.9
jenlamptonup
Comment #30.10
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.11
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.12
jenlamptontests
Comment #30.13
jenlamptonup
Comment #30.14
jenlamptonup
Comment #30.15
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.16
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.17
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.18
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.19
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #30.20
jenlamptonup
Comment #30.21
c4rl CreditAttribution: c4rl commentedUpdate issue list
Comment #30.22
c4rl CreditAttribution: c4rl commentedFix formatting :P
Comment #30.23
c4rl CreditAttribution: c4rl commentedsystem.module issue ready, updated summary.
Comment #30.24
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #31
jessebeach CreditAttribution: jessebeach commentedComment #32
jessebeach CreditAttribution: jessebeach commentedComment #33
jessebeach CreditAttribution: jessebeach commentedComment #34
jessebeach CreditAttribution: jessebeach commentedComment #35
jessebeach CreditAttribution: jessebeach commentedComment #36
jessebeach CreditAttribution: jessebeach commentedComment #37
jessebeach CreditAttribution: jessebeach commentedComment #38
jessebeach CreditAttribution: jessebeach commentedComment #39
jessebeach CreditAttribution: jessebeach commentedComment #40
jessebeach CreditAttribution: jessebeach commentedComment #41
jessebeach CreditAttribution: jessebeach commentedComment #42
jessebeach CreditAttribution: jessebeach commentedComment #43
jessebeach CreditAttribution: jessebeach commentedComment #44
xjmThe beta blocker #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system is postponed on this, which makes this critical and a beta blocker as well. If it gets close to beta and this isn't done, we might want to consider doing the rename first and not blocking the beta on cleanups here.
Comment #45
markhalliwellComment #46
joelpittetComment #47
martin107 CreditAttribution: martin107 commentedmoved install.core.inc from needs patch to needs review
Comment #48
martin107 CreditAttribution: martin107 commentedMove authorize.php up the totem pole ( from needs fix to needs review )
Comment #49
catchComment #50
martin107 CreditAttribution: martin107 commentedmoving items to fixed.
Comment #51
martin107 CreditAttribution: martin107 commentedform.inc moved to fixed.
Comment #52
martin107 CreditAttribution: martin107 commentedtablesort.inc moved to review by the community
looking at comment #1
grep -r " theme(" . | wc -l
358
Count now stands at
123
Comment #53
jessebeach CreditAttribution: jessebeach commentedComment #54
martin107 CreditAttribution: martin107 commentedRemaining - a review on 2 remaining NOVICE patches and a CRITICAL issue can be put to bed.
Comment #55
martin107 CreditAttribution: martin107 commentedAll issues In the hands of the core committers now
Comment #56
jessebeach CreditAttribution: jessebeach commentedComment #57
jessebeach CreditAttribution: jessebeach commentedWe missed one #2190427: Replace theme() with drupal_render() in form.inc. I patched it and tested it on
/node/add/article
.Otherwise any remaining instances of
theme()
are in comments or tests. I'll address the remaining comment instances in #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system.Comment #58
jessebeach CreditAttribution: jessebeach commentedComment #59
martin107 CreditAttribution: martin107 commentedcore/includes moved to fixed
Comment #60
webchickI just committed the last of these from the meta issue. WOOT!
But unfortunately, I found a few more. :(
It might be that the test-related ones need to stay there, but at least core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php and core/modules/views/lib/Drupal/views/Plugin/views/style/Rss.php should be looked into more.
There are a ton of other instances grep picks up, but they're all in comments, including out-dated ones that no longer make sense for the code below now that it's been converted to render arrays. The issue that deprecates theme() is definitely going to have its work cut out for it.
Comment #61
jessebeach CreditAttribution: jessebeach commentedComment #62
jessebeach CreditAttribution: jessebeach commentedAll sub-issus have been addressed. This critical is closed.
Comment #63
jessebeach CreditAttribution: jessebeach commentedComment #64
webchickSorry, but did you see #60? I'm not sure that's true, just yet...
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commentedyeah, this has been happening all along. see #29
maybe we should rename theme() to _theme() and then come back and revisit #60
Comment #67
pplantinga CreditAttribution: pplantinga commentedI agree with #65, we should rename theme() to _theme()
Comment #68
thedavidmeister CreditAttribution: thedavidmeister commentedComment #69
jessebeach CreditAttribution: jessebeach commentedWe've already renamed
theme
to_theme
. Is this what you are referring to?#2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system
Comment #70
pplantinga CreditAttribution: pplantinga commentedI just looked through a grep of _theme() and couldn't find any instances that shouldn't be there. Someone else want to verify?
Comment #71
catchYes that's it's own issue, that is either fixed or has one sub-issue left.