Move all the logic out of index.php (again)

Yes, +1 to this. Putting the logic on \Drupal works for me, I can't think of a better place to put it.

One question, is "frontController" the best we can do? It makes sense but at the same time, it's different to how any other "controller" works in core, eg a class.

Couple of doc things;

1. +++ b/core/lib/Drupal.php
   @@ -628,4 +633,45 @@ public static function accessManager() {
   +   *   The class loader. Normally Composer's ClassLoader, as included index.php,
   +   *   but may also be decorated; e.g.,
   

   "as included in index.php"

2. +++ b/core/lib/Drupal.php
   @@ -628,4 +633,45 @@ public static function accessManager() {
   +      // Set the response code manually. Otherwise, this response will default to a
   +      // 200.
   

   Longer than 80chars.

I think we had this a while ago (as a function) but it was reverted at some point.

Yes, we had drupal_handle_request() in bootstrap.inc. I don't really care either way, as long as the code can be autoloaded.

@#2: "Front controller" is what the text books say we should name it:

from Wikipedia:

"The Front Controller Pattern is a software design pattern listed in several pattern catalogs. The pattern relates to the design of web applications. It "provides a centralized entry point for handling requests." Front controllers are often used in web applications to implement workflows."

/Drupal::frontcontroller should be our centralized entry point.

`frontController` is a noun, which implies that it is an object, or provides access to an object. Nearly everything else in that class does this, and the docblock does say that it is the "Static Service Container wrapper." Functions that don't provide access to objects, like ::l() and ::url() are really just helper functions for the objects.

`\Drupal\Core\DrupalKernel` may be a better place for this, and may also give us a chance to refactor that class / interface into something a little cleaner, and provide a middleware to bootstrap in a different manner if it wants to. I would also suggest `\Drupal\Core\Drupalkernel::processRequest()` and also returning the $response object that gets created.

With the current approach it is possible to wrap the application kernel from within the front controller script. Not sure whether this is relevant, now that we have stack middlewares, but they wrap the http kernel, not the application kernel.

Maybe introduce DrupalFrontController which implements the KernelInterface and TerminableInterface

$autoloader = require_once __DIR__ . '/core/vendor/autoload.php';
$front_controller = new DrupalFrontController($autoloader, 'prod');

$request = Request::createFromGlobals();
$response = $front_controller->handle($request);
$response->send();
$front_controller->terminate();

Looking at Request::createFromGlobals() and Response::send() I do not expect them to throw exceptions which could be meaningfully caught with the If you have just changed code message. Exceptions thrown in terminate() cannot influence the output because at this point in time the response is already sent. Therefore we could move the messy try/catch into DrupalFrontController::handle() and even write unit-tests for it.

See also the Symfony SE front controller script.

I'll work on this if there is agreement on an approach. #8 seems like a good idea.

One common change in Drupal is to move the code out of Web root. The changes are not obvious. They could be made easier by defining the Drupal and Web roots at the start then documenting the change you can make when core is moved outside of Web root.

We do something similar in .htaccess with comments about changing redirections.

I suggest something like the following with a page somewhere on Drupal.org explaining how to change $drupal_root:

$web_root = __DIR__;
$drupal_root = __DIR__;
// $drupal_root = '/home/example/drupal';

... #8 code modified ...
$autoloader = require_once $drupal_root . '/core/vendor/autoload.php';

Déjà vu!

This was previously done earlier in the Drupal 8 cycle (#1831998: Reduce index.php for easier upgrading) but then was slowly whittled away and eventually removed. Given that history, I would suggest adding a code comment to index.php here explaining why we put no actual logic directly in the file - then maybe it will stick this time.

I would also suggest `\Drupal\Core\Drupalkernel::processRequest()` and also returning the $response object that gets created.

That makes more sense to me too, or possibly handleRequest()? - keeping in mind that the earlier issue that did this called it drupal_handle_request(). Either "handle" or "process" sounds good to me.

I got bored last night, and wrote up a quick patch. I started down the road in #8, but it became apparent that 95% of KernelInterface wasn't needed, so I made DrupalFrontControllerInterface / DrupalFrontController and wired it up that way. Besides, it keeps the kernel doing kernely things, and the front controller as just the front controller. Just waiting for TestBot to give the OK of the POC patch.

I believe we should make it easy to use DrupalKernel in modern code. Therefore let's do the following:

• Make DrupalKernel::createFromRequest() optional, i.e. move the initialization code to DrupalKernel::handle(). In the long run we probably should get rid of DrupalKernel::createFromRequest() completely.
• Also move the try/catch block from the front controller to DrupalKernel::handle().

That makes it possible to use the following standard pattern in intex.php (and also http[s].php):

$autoloader = require_once __DIR__ . '/core/vendor/autoload.php';

$kernel = new DrupalKernel('prod', $autoloader);

$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->prepare($request); // <- Edit: removed in #14
$response->send();

$kernel->terminate($request, $response);

As a consequence some of the setup code now is duplicated in prepareLegacyRequest(). On the other hand, the installer-redirection logic now is inside the standard request-response flow. I think we do not need that logic in legacy requests anyway.

Fix run-test.sh, also Response::prepare() is not expected to be called from the front controller.

Had this done before @znerol posted #13, but was waiting to see if anything major came out of TestBot before I posted it to the issue. I think this will come up green, and the exception handling works with my test scripts.

#13 has some static vs object context problems, so I couldn't try the HTTP exception handling with it, but it looks like the better thing to do.

+++ b/index.php
@@ -8,38 +8,13 @@
+// Craete a DrupalFrontController object and use that to process the request

Create is spelt wrong :) Looks pretty good to me.

Discussed this with @dawehner and @mpdonadio in IRC, conclusion is that we want to go with approach in #14, i.e. the front controller script should instantiate a kernel. Test failure in ConfigExportUITest is due to adventurous global tainting code in config_export_test. This is easy to rewrite properly. Interdiff is against #14.

#21 contains debugging stuff. Rerolling, interdiff is still against #14.

Bumped into #2409247: Generated proxy classes break DrupalKernelTest when running from the UI while trying to fix the test failures in DrupalKernelTest. Let's roll back the changes to that test for the moment.

+++ b/core/modules/config/src/Tests/ConfigExportUITest.php
index d0ceccd..0000000
--- a/core/modules/config/tests/config_export_test/config_export_test.info.yml

I think this is rebase gone wrong.

No, just an incomplete interdiff ;)

Let's try to reduce code changes such that its not necessary to touch tests.

Opened #2409547: Remove config_export_test.module

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -749,6 +769,10 @@ public static function bootEnvironment() {
+    $core_root = dirname(dirname(dirname(__DIR__)));

Can't we use app variable here? Or is it not available yet?

Can't we use app variable here? Or is it not available yet?

bootEnvironment() is static, so regrettably no.

This needs a reroll due to #2221699: HTTP_HOST header cannot be trusted. I'll take care of it since I still have my testing scripts handy for checking the things that can't be handled by unit/web testing.

All this sounds like a regression to me.

@znerol:

I believe we should make it easy to use DrupalKernel in modern code. Therefore let's do the following:

• Make DrupalKernel::createFromRequest() optional, i.e. move the initialization code to DrupalKernel::handle(). In the long run we probably should get rid of DrupalKernel::createFromRequest() completely.
• Also move the try/catch block from the front controller to DrupalKernel::handle().

DrupalKernel::createFromRequest() is that way for a reason: our whole processing, the kernel, the container, etc. all depends on the settings, and as a consequence all depend on the request. This is not "old" code (as opposed to "modern"), it is a conscious architecture decision.

If we want to reduce the amount of code in the front controller, the answer is to add a convenience function somewhere, not to butcher the architecture of the kernel.

@damz, the patch in #15 is essentially what you described; a helper object rather than a helper method, though.

There was discussion this weekend about which approach was better on IRC, and we decided on this. I forget who was in on it, though.

Think this is good, and should come up green.

@Damien Tournoud: The very goal of this issue is to get rid of the try...catch in index.php. This is only possible if the front controller only calls code that either does not throw exceptions or is capable of handling them.

Several approaches have been proposed and explored:

1. Copy paste the whole block into a static function in /Drupal (#1, #6)
2. Introduce a DrupalFrontController class #8 (implemented in #15)
3. Pack the logic into DrupalKernel::handle() #13ff

Option 1 is essentially what was there prior to the kernel-bootstrap patch but additionally removes the exception handling from the front controller. Whether or not to bury everything into one static function was discussed during review of said patch and it was a conscious architecture decision to better, bring our front controller closer inline with a Symfony kernel app. (@neclimdul #2016629-227: Refactor bootstrap to better utilize the kernel)

Option 2 is only marginally better in the current implementation. It would be acceptable if DrupalFrontController would implement the Symfony HttpKernelInterface and the TerminableInterface.

Option 3 just moves some bits of the Kernel around such that there is no need for DrupalFrontController at all. My statement about deprecating DrupalKernel::createFromRequest() was a bit premature, because we still need it in various places where it would be inconvenient to follow the front controller pattern.

@znerol: Given how DrupalKernel is currently structured, only one site path can be handled in a given instance.

Changing this to allow multiple independent requests to be processed (potentially at the same time) by a single instance of DrupalKernel is a relatively large undertaking, that is extremely far from being done by the current patch.

Looking at the current architecture, it is not even possible to do so properly, because several methods on DrupalKernelInterface are bound to a specific site anyway (->getServiceProviders, ->discoverServiceProviders, ->updateModules, ->handlePageCache, etc.).

That puts option #3 completely off the table. I suggest we go with #2, which sounds like the cleaner given the current state of the DrupalKernel architecture.

Given how DrupalKernel is currently structured, only one site path can be handled in a given instance.

I completely agree. Neither of the three approaches attempts to change that. I assume that you misunderstood option #3 somehow. In fact it does the same as #2 but without introducing an additional front controller class.

@znerol: I understood option #3, and I'm saying it's not a practical option at all. The current patch tries to make DrupalKernel handle multiple independent requests and it is utterly insufficient.

Just as an example, in the implementation from #36, ->handle calls ->initializeSettings, which is going to detect the site path based on the request, but none of the rest of the call is actually re-entrant. If ever the site path changes between two requests, things are going to be utterly broken, because the container doesn't support that: the container is not going to be booted again, so you are going to end up with a new site path, but still the old list of modules, the old container, etc.

Let me re-iterate: Given how DrupalKernel is currently structured, only one site path can be handled in a given instance.

Changing that (which #3 is trying to do) is hard, and I suggest option #2 (adding a multi-site DrupalFrontController in front of the current DrupalKernel) is a better idea at this stage.

Thanks for the clarification and the example. It seems to me that reusing the application kernel for multiple requests is a pretty exotic thing to do. I've never encountered a Symfony application which actually tried this. On the other hand, reusing the same http kernel for subrequests is quite common and that's also supported by Drupal.

That said, it is actually easy to protect against the scenario presented in #40. The only thing to do is to throw an exception if anyone tries to set the site path on a booted kernel.

Finally let me point out that even DrupalKernel::createFromRequest() does not protect against crazy people attempting to handle() different requests with the same Kernel.

It's like the tiniest nitpick ever, but here it goes. I hope it gets the issue going again. I would have really liked this in beta5/6. It would have given me more confidence in planning out the composer approach for building a site I am planning.

Isn't that now a duplicate of an issue tstoeckler was working on recently and got rtbc?

@dawehner: Which one do you mean? #2406681: Add an autoload.php in the repo root to control the autoloader of front controllers? I do not see any significant overlap there.

The front controller should grab the global and environmental parameters and inject them into the kernel.

The kernel should be an object that encapsulates all the behavior, but which requires that you inject parameters.

Basically the kernel is the object that forces you to start the injection of parameters for everything else.

Hence:

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -565,8 +541,46 @@ public function terminate(Request $request, Response $response) {
   +        $rebuild_path = $GLOBALS['base_url'] . '/rebuild.php';
   

   Superglobal in the kernel. Nope. :-)

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -743,6 +757,10 @@ public static function bootEnvironment() {
   +    // Include our bootstrap file.
   +    $core_root = dirname(dirname(dirname(__DIR__)));
   +    require_once $core_root . '/includes/bootstrap.inc';
   

   __DIR__ and require_once in a static kernel method. Nope. :-)

@47:

Are you saying that you would want the base_url and the core_root to be injected?

The global is only used if something serious has gone wrong - and this generic exception catch completely breaks Drupal's exception handling so I think this should be removed too.

I disagree. The standard exception handling is not affected by this change because that happens in the http kernel. What we are doing here is moving the stuff from index.php one layer down to DrupalKernel (the application kernel).

Rerolled.

@alexpott: I do not quite understand. Does this patch alter the behavior in this regard?

Why is the prepare() being moved inside the Kernel? I don't think I've ever seen that done inside an HttpKernelInterface implementation. (Although I admit I haven't surveyed all Symfony-using projects.) One method call is not a big expectation for the rare person writing a new front controller.

Symfony SE front controller script, Symfony HTTP Cache Kernel, laravel front controller script.

IMHO prepare() clearly belongs into the application kernel, and not the front controller script.

Also note, that in Symfony prepare() is called from a response listener for uncached requests.

Interesting. That must have changed at some point because that didn't used to be the default in Symfony SE. I withdraw my objection.

DrupalKernel::createFromRequest changed. The class loader stuff probably should go into the new initializeSettings() method.

I think this is at least major. If we ship with the current crap-loaded front-controller then sites will have a hard time to customize it in a sane way.

Uhm, that was the wrong patch. Ignore #64, interdiff is against #61.

Interesting. Seems like TestDiscovery also should be blowing up in HEAD.

Filed #2474817: DrupalKernel::classLoader not updated when switching to apcu either through settings.php or automatically.

Reroll b/c #2474817: DrupalKernel::classLoader not updated when switching to apcu either through settings.php or automatically is in, but this take care of that already. Or am I misreading #66?

Weird. #72 looks fine. I cannot reproduce the test failures on my local machine.

Reupload.

use Drupal\Core\DrupalKernel;
use Symfony\Component\HttpFoundation\Request;

$autoloader = require_once 'autoload.php';

$kernel = new DrupalKernel('prod', $autoloader);

$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();

$kernel->terminate($request, $response);

Ahhhh. :) Much better.

Reroll

I think we use LogicException in some other places.

Oh, right!

Here is a question, maybe its just dump.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -778,6 +775,10 @@ public static function bootEnvironment() {
+    $core_root = dirname(dirname(dirname(__DIR__)));
+    require_once $core_root . '/includes/bootstrap.inc';

So we have two places where we call bootEnvironment from: ::handler() and ::createFromRequest(), but both places have a kernel $kernel already. Given that I'm curious why bootEnvironment has to be static, because in case it isn't we could use $this->root instead.

bootEnvironment() is also called statically from within rebuild.php and I do not see an obvious way on how to fix that.

Mh right, I just had a look at the patch file for itself, too bad.

I like the general work forward.

Reroll.

Proper conflict interdiff.

<?php

/**
 * @file
 * The PHP page that serves all page requests on a Drupal installation.
 *
 * All Drupal code is released under the GNU General Public License.
 * See COPYRIGHT.txt and LICENSE.txt files in the "core" directory.
 */

use Drupal\Core\DrupalKernel;
use Symfony\Component\HttpFoundation\Request;

$autoloader = require_once 'autoload.php';

$kernel = new DrupalKernel('prod', $autoloader);

$request = Request::createFromGlobals();
$response = $kernel->handle($request);
$response->send();

$kernel->terminate($request, $response);

Aw yeah.

Hey, this looks awesome. I don't want to sidetrack things but I have a question that relates to the work here.

Would it be possible / crazy to convert the string ('prod') to a constant in this line:

$kernel = new DrupalKernel('prod', $autoloader);

That way we could pull index.php out of the git repo and have it vary for different environments. Does that make sense?

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -213,53 +214,9 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +    $kernel->initializeSettings($request);
   
   @@ -576,8 +536,46 @@ public function terminate(Request $request, Response $response) {
   +      $this->initializeSettings($request);
   

   I'm not sure why we're keeping createFromRequest still. If you use it though you'll hit settings initialization twice with is fairly expensive.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -576,8 +536,46 @@ public function terminate(Request $request, Response $response) {
   +    // Adapt response headers to the current request.
   +    $response->prepare($request);
   

   Shouldn't this stay in the front controller

Sorry, I got distracted and forgot to follow up and on reading the issue on the prepare.

I disagree with moving it still. A listener and hard coded into the kernel have different effects on modularity and testing. Moving has no real benefit either.

@cosmicdreams: Would it be possible / crazy to convert the string ('prod') to a constant in this line:

I'm pretty sure this isn't used anywhere other than perhaps simpletest, which has its own kernel subclass anyway.

I'd love to see the environment hooked into something useful, so we can support lightweight functional testing with fixtures, but that's out of scope here, too.

If you have a use-case in mind, create a follow-up, please. :-)

I disagree with moving it still. A listener and hard coded into the kernel have different effects on modularity and testing. Moving has no real benefit either.

Not sure about the benefit of that. In which case do we want to swap that out?

A case to consider is non-Drush / non-Console CLI applications, (e.g. Beanstalkd queue runner), for which anything HTTP-related is entirely irrelevant, and need to build fake headers for the CLI "request" in their own bootstrap.

The less HTTP they have to fake, the simpler and more robust it is for these.

Re #94.1:

I'm not sure why we're keeping createFromRequest still. If you use it though you'll hit settings initialization twice with is fairly expensive.

We use that in the following cases for legacy scripts and in tests. In both cases it is desirable to initialize the settings.

$ git grep -l DrupalKernel::createFromRequest
core/authorize.php
core/modules/simpletest/src/BrowserTestBase.php
core/modules/simpletest/src/InstallerTestBase.php
core/modules/simpletest/src/WebTestBase.php
core/modules/statistics/statistics.php
core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
core/scripts/password-hash.sh

Given that createFromRequest() did not change in this patch but merely DrupalKernel::handle(), the important question is whether the latter method is used anywhere on an application kernel instance created with the static factory instead of directly with constructor. In order to find those cases, it is necessary to filter out occurrences where the http kernel is invoked (i.e. subrequests etc.).

$ git grep -i -- 'kernel.*->handle(' | grep -vEi 'http_?kernel'
core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php:    $response = $kernel->handle($request);
core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php:    $response = $kernel->handle($request);
core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php:    $response = $kernel->handle($request)->prepare($request);
core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php:    $response = $kernel->handle($request)->prepare($request);
core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php:    $response = $kernel->handle($request)->prepare($request);
core/modules/system/tests/http.php:$response = $kernel->handle($request);
core/modules/system/tests/https.php:$response = $kernel->handle($request);
index.php:$response = $kernel->handle($request);

The only questionable file ExceptionHandlingTest.php is a false positive.

There is nothing in core which calls DrupalKernel::createFromRequest() followed by handle(). The reason why the static factory is still necessary is its usage in legacy scripts and drush.

Re #94.2:

Shouldn't this stay in the front controller

The purpose of the Response::prepare() is to tweak

[...] the Response to ensure that it is compliant with RFC 2616. Most of the changes are based on the Request that is "associated" with this Response.

For example it removes the response body if the incoming request used the HEAD verb. It's obvious that this should not be done before storing the response in the internal page cache. Therefore a response subscriber would only work for us if the page cache is disabled otherwise another mechanism is necessary. It would be possible to add another early middleware in order to make it pluggable.

I still do not think it is necessary to make it pluggable. If there is a technical reason to do it, then I'd prefer to move the call back to the front controller, because adding a new middleware for that single function call seems like overkill.

@all: Especially those with Symfony experience, please help us decide whether there are situations where running $response->prepare($request) inside DrupalKernel::handle() would be wrong.

@znerol #99: There is nothing in core which calls DrupalKernel::createFromRequest() followed by handle(). The reason why the static factory is still necessary is its usage in legacy scripts and drush.

The first line of createFromRequest is this: $core_root = dirname(dirname(dirname(__DIR__)));. So automatically it's breaking isolation.

Semi-related: Reading over DrupalKernel right now it still kind of irks me that we can't inject DRUPAL_ROOT. The constructor says this: $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));. Makes me itch. We could test the kernel with vfsstream if this were to change.

I think those legacy scripts and drush need to get broken and re-factored a bit, but I beat my head against that when it *wasn't* beta time and no one cared, so I'm not holding out hope.

For example it removes the response body if the incoming request used the HEAD verb. It's obvious that this should not be done before storing the response in the internal page cache.

I'm not sure that's the case. The render process can change headers, and the headers can be cached. Also, if parts of the response are cached we don't need to rebuild them on the next similar request.

@all: Especially those with Symfony experience, please help us decide whether there are situations where running $response->prepare($request) inside DrupalKernel::handle() would be wrong.

DrupalKernel::handle() is a shim for HttpKernel::handle(). I prefer the thin shim approach for overridden methods like this.

Of course, if there's an overriding design need, then yay. So what's the overriding design need?

@fgm mentions CLI apps having needs, but really, those should be addressed in other ways. For instance, if there's a need for CLI clients to clear the cache or run cron in an easy way, then let's build CliKernel and give it methods like clearCache($type='all');

I'm not sure that's the case. The render process can change headers, and the headers can be cached. Also, if parts of the response are cached we don't need to rebuild them on the next similar request.

The PageCache middleware stores/delivers the whole Response object including headers, status and content. The response event is emitted by the http kernel (i.e. the innermost kernel which is wrapped by several middlewares, including the page cache one). I.e., PageCache::set() is invoked with the result of HttpKernel::handle().

Response::prepare() adapts the response according to user agent characteristics and also removes the content if it was generated as a result of a HEAD request. Calling that before PageCache::set() would lead to a cache-poisoning issue. It follows that Response::prepare() cannot be in a response listener because that runs before PageCache::set() on a cache miss.

So really we have three options where to call that method:

1. In the front controller (like in HEAD, but that deviates from Symfony front controllers)
2. In the application kernel (patch)
3. Inside a new middleware with priority > than page cache

It would be great if we could stick to the scope of this issue and concentrate on picking the right choice here.

Thanks znerol for addressing my concerns. I'm not sure I agree about prepare() still but I'm not sure its worth blocking.

Doing one more review, I had one more question. Currently if I want to customize/replace/or remove the hard coded catch message I could replace the front controller but now you have to replace DrupalKernel or patch it. Neither option is very reasonable. Should that not either be in the front controller or configurable in some way?

Should that not either be in the front controller or configurable in some way?

The point of this issue is to remove the try...catch from the front controller. What about another setting?

I was actually mindful of scope when bringing it up but this would be a regression. Also the scope of this issue was to make maintenance and upgrades easier but adding the requirement of possibly having to maintain an alternative kernel or patching core is worse then the current situation.

Exceptions caught here are those which are thrown before our exception handler is installed (and before there is a container). We could add an error_log() though.

adding the requirement of possibly having to maintain an alternative kernel or patching core is worse then the current situation.

Could you please provide a use-case where this would be necessary with #103.

I'm not sure that is a problem. $catch is actually true by default and the $catch logic is actually run after the logging would have happened. In that respect it should be exactly the same as HEAD only one method call removed from its original location. Its the last thing run in the last handle method.
You would set it to FALSE to catch all exception in your front controller for what ever reason. Debugging or very specific logic I guess. Side note, I tried to rationalize this as an option instead of using a setting but its the behaviour of all exceptions all the way up the stack-middleware tree though so I think it would be fairly ineffective at targeting something specific like the "rebuild" message.

Exceptions caught here are those which are thrown before our exception handler is installed.

Ok, that's not quite true. Exception caught here are those which the exception handler fails to handle.

You would set it to FALSE to catch all exception in your front controller for what ever reason.

You set $catch to FALSE in unit tests. This is the one and only reason for that parameter. Perhaps it would be clearer if it would be named $return_response_whatever_it_takes = TRUE (this is what we want in production).

Yeah, alex corrected me on IRC. I don't know what I was thinking.

His concern then is actually probably legitimate then because if a random page(not the entire site) is throwing an exception its going to probably slip through without the logging.

Try this experiment. Turn off the database, add a breakpoint on Drupal\Core\Database\Driver\mysql\Connection::open() where the PDO object is constructed (line 92) and then step through the exception handling.

HEAD: The throw at the end of the catch block in index.php sends us to _drupal_exception_handler(), that throws again because there is no Database (and therefore no config) and sends us back to the front controller and then again back to the error handler and so fort until finally something breaks that loop - I have no idea what.

Because we do not throw again in the patch we do not enter that feedback loop. Alex is right indeed that in that case we side-step the exception handler. I think it is wrong to rethrow if $catch is FALSE, on the other hand we probably want to give the exception handler a chance to log something if possible. So perhaps we should simply forward to the exception handler from the catch clause and only add the rebuild message if that throws also.

Filed #2483587: Bootstrap errors masked due to Drupal::hasService only checking for presence of a service, not whether it is actually initialized

Let's extract the exception handling code into its own method and give the Drupal exception handler a chance to log it.

Setting this back to Needs Review based on #114 being green after the re-queue from the testbot sadness today.

I manually tested both the hostname length protection and the trusted host mechanism, and both triggered the proper path through DrupalKernel::handleException()

Let's be bold, and see if this addresses alexpott's concerns.

Reroll after #2491155: Update drupal.org and kernel.org URLs in core modules (Follow-up to 2489912).

Blindly re-RTBCing after a successful reroll...

w00t !! Awesome !