Render the maintenance/install page like any other HTML page

My latest & greatest trick to further simplify the early installer environment:

    // Various services need a database connection. Supply one.
    if (!$install_state['database_verified']) {
      Database::addConnectionInfo('install', 'default', array(
        'driver' => 'sqlite',
        'database' => ':memory:',
      ));
    }

:-)

I... have no idea how this patch manages to do this, but it only occurs with Standard profile:

This backtrace clarifies it:

#0  Drupal\user\Form\UserLoginForm->__construct() called at [\core\modules\user\lib\Drupal\user\Form\UserLoginForm.php:67]
#1  Drupal\user\Form\UserLoginForm::create() called at [\core\lib\Drupal\Core\Form\FormBuilder.php:156]
#2  Drupal\Core\Form\FormBuilder->getFormId() called at [\core\lib\Drupal\Core\Form\FormBuilder.php:187]
#3  Drupal\Core\Form\FormBuilder->getForm()
#4  call_user_func_array() called at [\core\includes\form.inc:116]
#5  drupal_get_form() called at [\core\modules\user\lib\Drupal\user\Plugin\Block\UserLoginBlock.php:35]
#6  Drupal\user\Plugin\Block\UserLoginBlock->build() called at [\core\modules\block\lib\Drupal\block\BlockViewBuilder.php:47]
#7  Drupal\block\BlockViewBuilder->viewMultiple() called at [\core\modules\block\lib\Drupal\block\BlockViewBuilder.php:31]
#8  Drupal\block\BlockViewBuilder->view() called at [\core\includes\entity.inc:466]
#9  entity_view() called at [\core\modules\block\block.module:203]
#10 _block_get_renderable_region() called at [\core\modules\block\block.module:172]
#11 block_get_blocks_by_region() called at [\core\modules\block\block.module:126]
#12 block_page_build() called at [\core\includes\common.inc:3648]
#13 drupal_prepare_page() called at [\core\includes\install.core.inc:945]
#14 install_display_output() called at [\core\includes\install.core.inc:135]
#15 install_drupal() called at [\core\install.php:33]

block_page_build() kicks in, because Block module is installed by Standard profile.

This patch removes all the special-casing for the install page, so hook_page_build() of Block module is actually executed as soon as it is installed! :-)

And block.block.seven_login.yml contains:

region: content

So we have these options:

1. Intentionally cripple the installer environment to force only system + user modules to be active.
2. Don't invoke hook_page_build() in the installer.
3. Move block.block.seven_login.yml into a different region.
4. Don't allow the install profile to ship with content region blocks in the theme that is used in the installer.

3) + 4) are plain silly, so only 1) + 2) are sensible options.

My favorite would be (1), but I already guess that the module list is actively used for much more when modules are installed (e.g., to discover/install config for other modules).

But let's try that first:

Intentionally cripple the installer environment to system + ?user + ?profile modules.

OK, that ends with:

Fatal error: Uncaught exception 'Doctrine\Common\Annotations\AnnotationException'
with message '[Semantical Error] Couldn't find constant COMMENT_MODE_THREADED, class Drupal\comment\Plugin\Field\FieldType\CommentItem.'
in \core\vendor\doctrine\annotations\lib\Doctrine\Common\Annotations\AnnotationException.php on line 52

Thus, only option (2) remains. Which is essentially the same as HEAD.

However, HEAD bakes that special case into template_preprocess_maintenance_page(), which is invoked at the very end of the page rendering chain and thus that doesn't work with this patch. Instead, we need to cover the special case directly in drupal_prepare_page().

1. Revert "Intentionally cripple the installer environment to system + ?user + ?profile modules."
2. Only invoke hook_page_build() of System module in MAINTENANCE_MODE.

But now... I'm unable to reproduce that installation failure reported by the testbot.

Installation with Standard profile works perfectly fine for me.

Another incident of #1808220: Remove run-tests.sh dependency on existing/installed parent site ?

@tstoeckler: Nope, forcing all installation profiles to ship with a separate theme for the installer would be crazy. ;-) Typically the installer appearance should follow the overall experience and just loads some additional asset libraries.

@dawehner:

1. I was surprised to see that we're not actually registering the classloader namespaces for the minimal bootstrap modules in the early installer environment. We did not notice that lack yet, because the early installer pages are solely driven by callbacks in install.core.inc only thus far. — Yes, normally DrupalKernel takes care of registering the modules, but we are manually overriding the module list. — In light of this, I wonder whether these manual module list overrides aren't architecturally broken... IMHO, such code should call a method on DrupalKernel, instead of manually futzing with ModuleHandler. → Speaking of, all we're missing is a call to DrupalKernel::updateModules().
2. The change to _drupal_render_process_post_render_cache() is debatable, since assigning non-existing callback should normally blow up... Therefore, I'd like to revert that. It's also a bit out of scope, as we've fixed the classloader registration already.
3. Regardless of the classloader issue, it looks like we simply cannot invoke hook_page_build() for the maintenance page and install page. Therefore, my suggestion is to simply wrap the invocation into if (!defined('MAINTENANCE_MODE')).
4. The SQLite memory database workaround was meant as a temporary trick for now, since I wanted to focus on the actual meat, instead of resolving silly container dependencies. ;) I'm not sure whether that can or will stay. The testbots definitely have pdo_sqlite. But perhaps this is why the testbot fails to install...
5. I really appreciate the attempt to modernize the installer some more by returning proper responses! However, I'd like to omit/revert those changes in this patch, since I'm trying very hard to limit the scope as much as possible — the current patch contains a lot of major changes to the install system and theme system already, and as we can see, it is hard enough to make just those work... — There's nothing wrong with the additionally proposed changes, but let's do them in a follow-up issue to this one here, pretty please. :-)

Let's see how this performs:

1. Properly update the module list on DrupalKernel.
2. Correction: Properly boot the kernel with the appropriate module list.
3. Do not invoke hook_page_build() in MAINTENANCE_MODE.
4. Replaced SQLite workaround with MAINTENANCE_MODE conditions in template_preprocess_page().

Interdiff is against #7.

1. Updated maintenance page templates accordingly.
2. Reverted error/exception handler override for installer.
3. Cleanups to minimize changeset.

Alright... let's further strip this down until we're able to pinpoint the testbot installation problem.

(btw, the recent testbot runs were based on PIFR 2.x and thus did not use drush site-install but manually walked through the interactive installer instead.)

1. Reverted all changes to install_begin_request().
2. Change template_preprocess_page() + drupal_prepare_page() to not call into menu.inc functions in MAINTENANCE_MODE.

At least we have test results now :-)

This is ugly, but the current legacy code paths do not leave much room for a cleaner alternative right now:

Added *temporary* DefaultHtmlPageRenderer::renderPage() helper, in order to move forward.

re #23: The maintenance page is rendered by all of the special front controllers (install.php, update.php, authorize.php, etc), which are not converted to a kernel + request/response architecture at all yet. — Converting (rewriting) those is out of scope here, so we need a temporary helper function that allows to render a maintenance/install page.

But even that is a step forward; centralizing all those special cases to call a single helper function will help us to convert those special front controllers.

1. Fixed bogus $theme parameter in install_display_output().
2. WTF: menu_local_tabs() returns no tabs unless menu_get_local_actions() is called upfront.

1. Attempt to update RenderElementTypesTest::testMaintenancePage().
2. Removed obsolete maintenance page test introduced by #2072647: #theme 'maintenance_page' should support render arrays in #content

Merged 8.x.

Resolving the last test failures:

Fixed bogus variable name in update_check_requirements().

Not going to fix the InstallerTranslationTest failures, because those test assertions will be removed entirely with #318975: Remove confirmation page after installation, which is RTBC already.

The only remaining @todo that is also added in code by this patch is whether we want to try to rename the templates as part of this patch?

maintenance_page.html.twig → page-maintenance.html.twig
install_page.html.twig     → page-install.html.twig

I'd prefer to do that in a separate follow-up issue, but could be convinced to do it here if someone comes up with a good reason for doing so.

31: theme.maintenance.31.patch queued for re-testing.

Yay! :-)

Since the new code mostly goes through the regular HTML page rendering pipeline, no special testing is required for this patch.

In fact, this patch removes a test for #theme maintenance_page, as mentioned in #26, because that test is pointless now.

From my perspective, this patch only needs technical reviews, because the UI is not changed at all (aside from adding the standard "Skip to content" link that we discussed already.)

The changes to drupal_prepare_page() could potentially be reverted, because that function is not invoked anymore. At the same time, that function only seems to get invoked from very similar edge-cases (e.g. BatchController), so we might as well keep those changes (and/or just don't care).

hook_page_build() is intentionally not invoked from the new helper method. I already considered to document that, but how do you document that some non-existing code is not executed, because you intentionally did not include/call the code? :P — j/k, I'll add some.

In any case, the new helper method needs reviews, and also, I'm not happy about the amount of added !defined('MAINTENANCE_MODE') conditions, although I don't think we can do much about the latter.

Updated for new .maintenance-background HTML class in Seven theme.

1. Reverted changes to drupal_prepare_page().
2. Added docs to DefaultHtmlPageRenderer::renderPage().

39: theme.maintenance.39.patch queued for re-testing.

Merged 8.x.

As this happens to affect the (critical) #1067408: Themes do not have an installation status, I guess we should at least bump this to major?

There are two conflicting expectations:

1. hook_preprocess* is invoked for a page.
2. On the update.php page, all hooks except for an explicit whitelist are not allowed.

→ If any hook_preprocess implementation calls any hook that isn't allowed, update.php blows up.

hook_preprocess* itself is allowed. But rdf_preprocess_html() tries to invoke hook_rdf_namespaces(). Thus, when visiting update.php when RDF module is enabled, then the exception is triggered.

Due to the conflicting expectations, I don't really have a proposal for how to resolve this.

As a matter of fact, it's pure coincidence that this patch revealed that problem. The same code path can be triggered through plenty of other mechanisms; e.g., #pre_render/#process/#after_build callbacks on form elements (that happen to be used on the update.php page), and most likely a lot more call chains. Just invoke a hook that happens to be not allowed in any of these, ka-boom.

We can't easily prevent the theme system from invoking preprocess functions in modules, because the "offline theme registry" (formerly used on the maintenance page) was removed a long time ago.

I'm aware why that hook limitation for update.php was introduced, but I'm not sure whether it is still necessary...?

IMO, this regression is a completely separate problem space — in essence, this change here is not flawed; it's the change that introduced that hook limitation that is flawed; because as we can see, the idea does not work out.

This patch only revealed a problem with the current update.php environment. The changes of this patch itself are fine on their own; there's nothing this patch could do to fix a problem that it didn't introduce. But yes, of course, we want update.php to work :-)

Created #2233403: Let update.php invoke hooks again

41: theme.maintenance.41.patch queued for re-testing.

#2233403: Let update.php invoke hooks again is resolved.

1. Merged 8.x.
2. Updated for changed update_task_list().

Doesn't appear to have been committed/pushed.