The theme_install_page() function simply calls drupal_add_http_header('Content-Type', 'text/html; charset=utf-8'); and then returns the maintenance page.

Additionally, it is identical to theme_update_page.

Attached patch removes theme function, and relocates functionality.

Related:
#1885850: Remove theme_update_page()
#2177655: Replace theme() with drupal_render() in install.core.inc

Files: 
CommentFileSizeAuthor
#18 1885714-18-proof-of-concept.patch9.68 KBjamesquinton
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,606 pass(es), 656 fail(s), and 105 exception(s). View
#16 1885714-16-proof-of-concept.patch6.92 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 64,632 pass(es). View
#13 Screen Shot 2014-02-08 at 09.58.12.jpg85.17 KBLewisNyman
#12 core-remove-theme_install_page-1885714-12.patch7.16 KBchakrapani
PASSED: [[SimpleTest]]: [MySQL] 63,782 pass(es). View
#2 1885714-2.patch3.08 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,897 pass(es). View
#2 interdiff.txt966 bytesCottser
core-remove_theme_install_page.patch2.73 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 49,495 pass(es). View

Comments

jenlampton’s picture

Issue summary: View changes

clarify, cleanup

jenlampton’s picture

Issue summary: View changes

link to identical theme func

nod_’s picture

Issue tags: +JavaScript

tag

Cottser’s picture

FileSize
966 bytes
3.08 KB
PASSED: [[SimpleTest]]: [MySQL] 53,897 pass(es). View

I don't think Seven should include mobile.install.js on all maintenance pages. I was able to get Seven to only include it on the install pages by using drupal_installation_attempted(), but that function returns FALSE on the "Installation complete" step so mobile.install.js is not included on the last step. Any suggestions appreciated.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Twig, -theme system cleanup

The last submitted patch, 1885714-2.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Twig, +theme system cleanup

#2: 1885714-2.patch queued for re-testing.

Cottser’s picture

Status: Needs review » Needs work
LewisNyman’s picture

LewisNyman’s picture

Issue summary: View changes

related

chakrapani’s picture

chakrapani’s picture

Assigned: Unassigned » chakrapani
Issue tags: +Needs reroll

The patch from #2 no longer works. Needs a reroll.
I will work on the re-roll.

chakrapani’s picture

Assigned: chakrapani » Unassigned
Issue summary: View changes
Status: Needs work » Closed (duplicate)
Issue tags: -Needs reroll

Most of the changes required for this are already committed to Head.
Only change required is

+++ b/core/includes/install.core.inc
@@ -872,7 +872,8 @@ function install_display_output($output, $install_state) {
-  print theme('install_page', array('content' => $output));
...
+  print theme('maintenance_page', array('content' => $output));

But this will be taken care by Replace theme() with drupal_render() in install.core.inc
Hence marking as duplicate and closing.

Cottser’s picture

Status: Closed (duplicate) » Needs work
chakrapani’s picture

Assigned: Unassigned » chakrapani

Looking into this.

chakrapani’s picture

Assigned: chakrapani » Unassigned
Status: Needs work » Needs review
FileSize
7.16 KB
PASSED: [[SimpleTest]]: [MySQL] 63,782 pass(es). View

Here we go. added the following changes to the patch:

  • removed call to install_page theme and changed them to call maintenance_theme
  • removed preprocess_install_page functions
  • removed install-page.html.twig files
LewisNyman’s picture

This breaks the installer design introduced in #1337554: Develop and use separate branding for the installer.

I'm not really sure how to achieve two designs while removing this theme function.

Also see: #2169765: Redesign update.php to be more consistent with the installation process

The installer looking a bit broken

chakrapani’s picture

Status: Needs work » Closed (won't fix)

After LewisNyman's comment and YesCT's suggestion over irc, I have looked into the issue again and noticed the following things:

  • The issue summary is outdated and theme_install_page() no longer exists
  • install_page no-longer calls the maintenance_page theme.
  • install page has a separate template "modules/system/templates/install-page.html.twig" which contains independent html compared to maintenance page.
  • install page also has its own install-page.css in the SEVEN theme.

Related Issues:
https://drupal.org/node/2041793
https://drupal.org/node/2169765

Marking this as won't fix after confirming with YesCT on irc.

Cottser’s picture

Status: Closed (won't fix) » Postponed

Once we have #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() this could potentially be a theme suggestion with a suggestion-specific prepare hook implementation. Postponing for now.

Cottser’s picture

Status: Postponed » Needs review
Issue tags: -JavaScript
FileSize
6.92 KB
PASSED: [[SimpleTest]]: [MySQL] 64,632 pass(es). View

Proof of concept patch that will ultimately be dependent on another issue (to be created). This shows two different methods of examining theme suggestion data in a preprocess function and preprocessing/preparing variables based on that information.

Fresh patch, so no interdiff.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Positive this needs a re-roll. Surprised I wasn't following it.

There is now a @todo in the template_preprocess_maintenance_page()
// @todo Rename the templates to page--maintenance + page--install.

This is very much in line with this idea and comes from #2218039: Render the maintenance/install page like any other HTML page

@Cottser how does this merge with your POC?

jamesquinton’s picture

Status: Needs work » Needs review
FileSize
9.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,606 pass(es), 656 fail(s), and 105 exception(s). View

Rerolled patch!

Status: Needs review » Needs work

The last submitted patch, 18: 1885714-18-proof-of-concept.patch, failed testing.

jamesquinton’s picture

Will take another look at this later...

Cottser’s picture

joelpittet’s picture

Status: Needs work » Closed (duplicate)

This function doesn't exist. Please move the POC over to a follow-up if you thing it's worth doing.