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.
Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
It might actually make sense to call theme()
in this file if common.inc
is not yet loaded. Be prudent.
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff-11-17.txt | 466 bytes | chakrapani |
#17 | drupal-core-replace-theme-with-drupal-render-2177655-17.patch | 1.83 KB | chakrapani |
#11 | interdiff-6-11.txt | 1.4 KB | martin107 |
#11 | drupal-core-replace-theme-with-drupal-render-2177655-11.patch | 1.83 KB | martin107 |
#6 | interdiff.txt | 2.44 KB | chakrapani |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedComment #2
InternetDevels CreditAttribution: InternetDevels commentedComment #3
InternetDevels CreditAttribution: InternetDevels commentedOops, missed # sign. Here is the corrected patch.
Comment #6
chakrapani CreditAttribution: chakrapani commentedRe-rolling the patch and Fixing the failing tests.
We should call theme maintenance_page through drupal_render rather than install_page theme similar to https://drupal.org/node/1885850
Comment #7
chakrapani CreditAttribution: chakrapani commented+needs review for the test bot to pickup..
Comment #9
star-szr6: drupal-core-replace-theme-with-drupal-render-2177655-6.patch queued for re-testing.
Comment #10
star-szrThanks @InternetDevels and @chakrapani!
We don't usually do "pretty arrays" like these, and the last item should have a trailing comma per http://drupal.org/coding-standards#array.
I'm not clear as to why this line is being added, was this to make tests pass? Because the tests that failed for #3 pass without this line for me.
Comment #11
martin107 CreditAttribution: martin107 commentedAs per #10 removal of the following line
drupal_add_http_header('Content-Type', 'text/html; charset=utf-8');
Plus application of coding standard to arrays
Comment #12
martin107 CreditAttribution: martin107 commentedassigned to me until it passes again.
Comment #13
martin107 CreditAttribution: martin107 commentedComment #14
pplantinga CreditAttribution: pplantinga commentedLooks good to me!
Comment #15
star-szrAs mentioned on IRC to @chakrapani this change (install_page to maintenance_page) seems out of scope and potentially dangerous here. Let's move this change to #1885714: Remove theme_install_page() please.
Comment #16
chakrapani CreditAttribution: chakrapani commentedHi Cottser,
I am working on this and #1885714
Comment #17
chakrapani CreditAttribution: chakrapani commentedRe-rolling the patch with suggestions in #15 by Cottser.
Now the patch purely changes theme() => drupal_render()
Comment #18
star-szrGreat, thanks!
Comment #19
jessebeach CreditAttribution: jessebeach commentedI ran a fresh install and found no rendering errors.
Comment #20
webchickCommitted and pushed to 8.x. Thanks!