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

CommentFileSizeAuthor
#1 drupal-core-replace-theme-2177637-1.patch670 bytesinternetdevels

Comments

internetdevels’s picture

internetdevels commented
Status: Active » Needs review
StatusFileSize
new drupal-core-replace-theme-2177637-1.patch670 bytes

Lets see...

Status: Needs review » Needs work

The last submitted patch, 1: drupal-core-replace-theme-2177637-1.patch, failed testing.

internetdevels’s picture

internetdevels commented
Status: Needs work » Needs review

1: drupal-core-replace-theme-2177637-1.patch queued for re-testing.

jessebeach’s picture

jessebeach commented 
+++ b/core/includes/ajax.inc
@@ -362,7 +362,8 @@ function ajax_prepare_response($page_callback_result) {
+    $commands[] = ajax_command_prepend(NULL, drupal_render($status_messages));

Is it possible to just pass the renderable array to the prepend command without calling drupal_render? Meaning, is drupal_render called on this argument later?

internetdevels’s picture

internetdevels commented

"Other general guidelines" part of META issue description says:

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.

So I'm not sure that we should pass renderable array here.

jessebeach’s picture

jessebeach commented

I guess that makes sense. We're not introducing a regression by replacing theme() in place. If we want further improvements, those can be introduced in another issue.

I would like to test this particular one further though, before I set to RTBC. Ajax is a tricky little monster in Drupal. Thanks for posting these patches InternetDevels!

jessebeach’s picture

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

Tested and this change is fine. It's small and innocuous.

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.