Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Title: Replace theme() with drupal_render() in instal.core.inc » Replace theme() with drupal_render() in install.core.inc
InternetDevels’s picture

Status: Active » Needs review
FileSize
1.83 KB
InternetDevels’s picture

Oops, missed # sign. Here is the corrected patch.

The last submitted patch, 2: drupal-core-replace-theme-with-drupal-render-2177655-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: drupal-core-replace-theme-with-drupal-render-2177655-3.patch, failed testing.

chakrapani’s picture

Re-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

chakrapani’s picture

Status: Needs work » Needs review

+needs review for the test bot to pickup..

Status: Needs review » Needs work

The last submitted patch, 6: drupal-core-replace-theme-with-drupal-render-2177655-6.patch, failed testing.

star-szr’s picture

star-szr’s picture

Thanks @InternetDevels and @chakrapani!

  1. +++ b/core/includes/install.core.inc
    @@ -986,9 +986,17 @@ function install_display_output($output, $install_state) {
    +    $task_list = array(
    +      '#theme'   => 'task_list',
    +      '#items'   => install_tasks_to_display($install_state),
    +      '#active'  => $active_task,
    +      '#variant' => 'install'
    +    );
    
    @@ -2529,7 +2537,11 @@ function install_display_requirements($install_state, $requirements) {
    +      $status_report = array(
    +        '#theme'        => 'status_report',
    +        '#requirements' => $requirements
    +      );
    

    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.

  2. +++ b/core/includes/install.core.inc
    @@ -986,9 +986,17 @@ function install_display_output($output, $install_state) {
    +  drupal_add_http_header('Content-Type', 'text/html; charset=utf-8');
    

    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.

martin107’s picture

As 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

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs work » Needs review

assigned to me until it passes again.

martin107’s picture

Assigned: martin107 » Unassigned
pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/install.core.inc
@@ -976,9 +976,16 @@ function install_display_output($output, $install_state) {
-  print theme('install_page', array('content' => $output));
+  $install_page = array('#theme' => 'maintenance_page', '#content' => $output);

As 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.

chakrapani’s picture

Assigned: Unassigned » chakrapani

Hi Cottser,
I am working on this and #1885714

chakrapani’s picture

Assigned: chakrapani » Unassigned
Status: Needs work » Needs review
FileSize
1.83 KB
466 bytes

Re-rolling the patch with suggestions in #15 by Cottser.
Now the patch purely changes theme() => drupal_render()

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

jessebeach’s picture

I ran a fresh install and found no rendering errors.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.