Refactor bootstrap to better utilize the kernel

thanks for this...

it'd still be helpful to have your unsuccessful approaches up somewhere, for our own digestion purposes.

from my brief look over of TestBase and WebTestBase, it seems rather like what we need to do is bite the bullet, rewrite the installer encapsulated within an InstallerKernel, then rewrite those methods to interact with the installation through that kernel by mucking with a request object.

yikes.

So my question given the current challenges I wonder if its impossible to at least make a patch available for advanced developers that do not need a web installer but who do need to integrate with other HttpKernelInterface applications and want to leverage stuff like stackphp.com? A semi official patch to make that happen might go a long way ..

x

installer left for 9.x #1530756: [meta] Use a proper kernel for the installer but there's sandbox https://drupal.org/sandbox/Letharion/1615168

installer left for 9.x #1530756: [meta] Use a proper kernel for the installer but there's sandbox https://drupal.org/sandbox/Letharion/1615168

What about using .htaccess to provide a second front-controller for simpletest.
And a second installer.

core/modules/simpletest/index.php
core/modules/simpletest/install.php

Then something like so

  # Redirect simpletest requests to simpletest.
  RewriteCond %{HTTP_USER_AGENT}  ^simpletest*
  RewriteCond %{REQUEST_URI} !^core/modules/simpletest/index.php$
  RewriteRule ^ core/modules/simpletest/index.php [L]

And for the installer

  RewriteCond %{REQUEST_URI} ^(.*)?/(install.php)
  RewriteCond %{HTTP_USER_AGENT}  ^simpletest*
  RewriteCond %{REQUEST_URI} !^core/modules/simpletest/install.php$
  RewriteRule ^ core/modules/simpletest/install.php [L]

Then we can break this circular dependency

How would that work for non-Apache web servers? We have an IIS config file we could probably modify the same way; I don't know about nginx.

nginx config is pretty well documented. it has easy equivalents apache/iis routing. this is a pretty intriguing proposal IMO.

in which case I might kick the tyres on this

Ok re-roll and refactor.
Let's iterate from here.
a) Manual install works.
b) run-tests.sh seems to work (tested a few tests and they passed).
c) Simpletest UI seems to work (tested a few tests and they passed too).

There is plenty to do and this doesn't remotely look like polished code.

From here I think we can continue this pattern.

Most of the drupal_bootstrap(SOMETHING)
become DrupalKernel::bootstrapSomething.

bootstrapSomething knows it needs to call bootstrapSomethingBeforeIt.

Once we get to bootstrapCode we need a kernel instance because we need to boot a container.
So we create, boot and return static::singleton from bootstrapCode.

I think from here we can start chopping out chunks from drupal_bootstrap and then start looking at using other statics/properties on DrupalKernel instead of the globals that simpletest has to mutate.

Lets see how borked this is from the bot's point of view, as I said - I ran some webtests locally and they passed without leaking into the main site, we're only moving stuff about so no reason why it should leak.

Drush doesn't work with this patch.

And in some places I'm getting Symfony\Component\DependencyInjection\Exception\InactiveScopeException: You cannot create a service ("finish_response_subscriber") of an inactive scope ("request"). in service_container_->getFinishResponseSubscriberService() but I think that's down to a botched re-roll because the original patch pre-dates the changes to request scope... but I can't see them in the diff.

when did testbot switch to using drush si?
what are the options here?
this breaks drush...

I suggested a different approach a long while back: https://drupal.org/comment/6757918#comment-6757918

I would like to see something like this:

$request = Request::createFromGlobals();

// Create the kernel from the request (because in our case, the container and kernel depend on the site, so they depend on the request.
$kernel = DrupalKernel::createFromRequest($request);

// Handle the response.
$response = $kernel->handle($request)->prepare($request)->send();

@larowlan - I also didn't know that bot used drush site-install. I thought we used this pift code. That indeed is problematic for installer patches like this. I was about to suggest working on #2115533: Add InstallerInterface to provide a stable non-interactive installer but I see you are already there. Nice!

I will add that if that issue gets stuck, there was a simpler proposal for pift to use its own small Drush command which would be much simpler that site-install, and would be maintained in core (and thus could move along with patches that change the installer). See #1317528: Separate the testbot from the installer.

If we end up needing help on the Drush side, I'm happy to pitch in.

That makes three of us.

more changes

New personal best!

more whittling

23: kernelify-bootstrap-2016629.23.patch queued for re-testing.

It would be great if someone could write a proper issue summary.

bah, language negotiation relies on variable system, so still need to boot that (another reason to go and review the language negotiation patch)

so changes made:

- adds a request subscriber for the theme negotiator - still calls out to the drupal_theme_initialize function but removes the call to setRequest on the theme.negotiator service, meaning we can drop drupal_bootstrap_full(). That stuff needs a good cleanup, lots of deprecated code including calls to drupal_add_js etc. Note this adds a $request parameter to it so we can call it from the request subscriber.
- boots the variable system after db and cache is available.
- fixes some uses of $_SERVER instead of the request in dblog

Hoping this will have less fails so then I can start the real cleanup work.

Also some of the code in here should be spun off into separate issues - eg it makes sense for conf_path() to take a $request parameter instead of using the super globals, same with the theme negotiator request subscriber.

meh drush en simpletest fails :(

stick back some BC stuff so drush keeps working

yeah, updated arg order of conf_path() to support drush but didn't revert changes made elsewhere

Updated issue summary

re-roll after language patch, lets see how we go now, still expecting theme fails

45 minute test run, must be my lucky day
fixes statistics fails, who knew we had a statistics.php, not I said the wolf

doh, forgot that priority works opposite to weight, so my theme init service was firing after PathSubscriber and admin theme wasn't being set because _system_path wasn't available on the request.

so this fixes admin theme, which should fix a lot of tests that look for specific markup (I'm looking at you HelpTest).

Also fixes warnings from dblog.

Fingers crossed for 1733 again, 45 mins is nice

Fixes http.php and https.php didn't know we had those either.
Logic there is funky at the moment but hoping in the final iteration we kill the testOnly concept, also hoping #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead helps here too.
Fixes any test with locale enabled - here's the problem:

* when locale isn't enabled calls to t() inside various hook_stream_wrappers() don't do much
* when locale is enabled the call to module_invoke_all('stream_wrappers') calls t() in various (system_stream_wrappers, locale_stream_wrappers et al)
* this calls into LocaleLookup which then in turn requires the current_user service
* which requires the synthetic request service

so I'm entering the request scope (which we have available thanks to DrupalKernel::createFromRequest) and it stops blowing up.

But it is pants, the request scope shouldn't be entered until we call DrupalKernel::handle(), at least thats what I reckon.

So hoping that #2028109: Convert hook_stream_wrappers() to tagged services. will help here too.

Is there a reason to call t() on name/description of file wrappers, they're such a low-level concept.

Anyway, lets see what bots says.

reroll after update.php was gutted.

variable_initialize() is dead

47: kernelify-bootstrap-2016629.47.patch queued for re-testing.

Re-roll after #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, go bot go

Missed some whilst rerolling, this fixes one of the failing tests locally, given the number of fails expect it will fix a lot of them

It'll fix Undefined variable: path in DrupalKernel.php.

I hate CollapsedDrupal\file\Tests\UsageTest test :@

55: kernelify-bootstrap-2016629.55.patch queued for re-testing.

slowly slowly down they came
each dog part of a long dog chain

What's the current state of this dog chain? :). Any blockers we should be working on?

Just need to find time..

re-roll

config.inc is gone

another re-roll

More fixes

needed another re-roll, velocity++

Fixed reference to drush as legacy. It isn't, just the function.

+++ b/core/includes/bootstrap.inc
@@ -234,33 +237,16 @@
+ * @deprecated see \Drupal\Core\DrupalKernel::confPath()

Deprecation messages should be specific about when the function is planned for removal, e.g.

@deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0. Use \Drupal\Core\DrupalKernel::confPath().

re-roll

I have simpletest test passing and know what the issue is with the theme and menu tests. But after five hours, I can't get the session https test to pass, although I've got closer than the last patch.
Can someone following this point we to where Drupal sets the cookie with the session name as manual testing suggests this cookie isn't getting set on login from the mock https bits of the test. I've stepped through both the mock login and a normal login and they seem identical yet for the mock https login the session cookie is never set. Also can someone confirm that https cookies even work in head as this test might be iffy too, and the changes are picking that up, I'm at a loss.

I searched for 'setcookie' and found a few instances that look like what you are wanting. See https://github.com/drupal/drupal/blob/f01c994de8d6f54a5bef1523319c6da880...

In my travels I also saw a warning against setting cookies on 'localhost domain name. See http://us2.php.net/manual/en/function.setcookie.php#73107.

I have never used https cookie so I can't easily help there.

@larowlan: if you look at the test log, the call to https.php still returns a status code of 500, which suggest that an exception was raised in the mock path, at least on the test bot. Do you know which exception that is?

Thanks Moshe and Damien
I had grepped for set cookie and had breakpoints on all instances in core. The code you've linked to is no longer in Drupal 8. I'm wondering if perhaps we're using symfony code to set the session cookie but I had breakpoints on those calls too.
Damien, I have another version locally where I changed the order of creating the request, like you suggested on irc, in not getting the 500 anymore. I'll upload that later today.
Thanks again

here's where I'm at, I think this should be sub-20 fails (famous last words)

whoops, had some stuff I'd taken out when working on the menu router/theme fails

#1316692: Convert hook_admin_paths() into declarative properties on routes is why the fails are up, need to investigate.

Ok, this should be enough to move back in the right direction after #1316692: Convert hook_admin_paths() into declarative properties on routes

And then there were 6.
5 fails in session https test, which I've tried in vain and at length to fix, any help appreciated.
1 in menu router test which I'll look at tonight after work.
Mental note, need to clean up the statics in the terminate() method so subsequent calls to ::createFromRequest get a clean kernel.

and the magic number is 29 - the theme init subscriber needs to run after routing (32) and after maint subscriber (30)

wrong patch

this works (at least locally)
also cleanup some outdated code

66,666 pass(es).

Satan!

I think that PHP is sending the cookie on its own, without any setcookie() call in Drupal. While trying to debug his, I put a breakpoint in curlHeaderCallback() and notice that we are in fact receiving a secure cookie back from Drupal but its name does not match $secure_session_name so the assertion fails. It seems odd to me that we call session_name() in the test (i.e. parent site) and expect that session name to be same on child site.

This test looks pretty brittle to me. It tries to emulate https without the webserver supporting https. This is a big enough hack that I'm not sure the test is really worth keeping?

Moshe: that was my conclusion too, but Paris managed to make it pass so now the real work can begin.

Impressive work. This is just a drive by review

1. +++ b/core/includes/theme.inc
   @@ -87,20 +88,20 @@ function drupal_theme_access($theme) {
   +function drupal_theme_initialize(Request $request = NULL) {
   ...
   -  $request = \Drupal::request();
   +  if (!$request) {
   +    $request = \Drupal::request();
   +  }
   

   Is there a reason why we can't just force to pass in the request?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -159,26 +250,60 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +   * @return \Drupal\Core\DrupalKernel
   +   *   Returns the built kernel.
   

   Small note: we could just use @return static

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -159,26 +250,60 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +   * @param \Composer\Autoload\ClassLoader;ClassLoader $class_loader
   

   small typo here.

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -202,12 +327,486 @@ public function getContainer() {
   +    if ($this->containerNeedsDumping && !$this->dumpDrupalContainer($this->container, static::CONTAINER_BASE_CLASS)) {
   +      watchdog('DrupalKernel', 'Container cannot be written to disk');
   +    }
   

   Is this really a situation when we want to trigger a watchdog call? This seems to be for me an exception

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -202,12 +327,486 @@ public function getContainer() {
   +  /**
   +   * Handle the request.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Request; $request
   +   *   The request.
   +   * @param int $type
   +   *   Request type.
   +   * @param bool $catch
   +   *   Catch errors.
   +   *
   +   * @return \Symfony\Component\HttpFoundation\Response
   +   *   The response.
   

   This is defined in the HttpkernelInterface so you could just get rid of this bit.

6. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -202,12 +327,486 @@ public function getContainer() {
   +   * @param \Symfony\Component\HttpFoundation\Request; $request
   

   Another small typo.

7. +++ b/core/lib/Drupal/Core/EventSubscriber/ThemeNegotiatorRequestSubscriber.php
   @@ -0,0 +1,72 @@
   + * Sets the $request property on the theme negotiation service.
   ...
   +      $request = $event->getRequest();
   +      $this->themeNegotiator->setRequest($request);
   

   You can easily get rid of this by using a synchronized request or even better a request stack. I guess the second one would be out of scope of this patch.

   In general though the important bit of this subscriber is the theme initialization.

8. +++ b/core/modules/statistics/statistics.php
   @@ -2,6 +2,14 @@
    
    /**
     * @file
   + * Provides statistics update callback.
   + */
   +
   +use Drupal\Core\DrupalKernel;
   +use Symfony\Component\HttpFoundation\Request;
   +
   +/**
   + * @file
     * Handles counts of node views via AJAX with minimal bootstrap.
   

   We need more @file statements :P

f

starts the real work, removes all calls to drupal_bootstrap().
so only drush left calling it (testbot).
doesn't address #102

ignores the settings() in interdiff, since fixed

hoping still green

adding beta target, cause planning to remove drupal_bootstrap().
fixed run-tests.sh

too aggressive on cleanup

go again, also think there's some fails in head, node translation ui or some such?

109: kernelify-bootstrap-2016629.109.patch queued for re-testing.

oom errors, hoping testbot

and again

Interdiff against 98.
Also fixes items in #102 except for #102.1 which is a much bigger issue and should be part of a larger theme system cleanup once this is in, in my opinion.
I spent an hour or so trying to make every usage of drupal_theme_initialize() pass in the request, its just a spaghetti mess, sometimes its even called when the request stack has no request object. So putting in the follow-up/out of scope basket.

Retains the terminate cleanup but doesn't address existing calls to drupal_bootstrap like patches 103->114 tried to do unsuccessfully. Going to do those in smaller steps to keep it green, tried for too much in one pass.

Also removes LegacyRequestSubscriber cause I don't see a need for it anymore

fixes cache and adds public bootPageCache method to replace _drupal_bootstrap_page_cache.
At the time this is called we have a booted kernel so we can continue to refactor that code to use the actual container ($this->container) instead of the globals/\Drupal object, but thats next pass.

shuffles stuff around to suit update.php

Green again
Ready for serious reviews now

1. +++ b/core/includes/bootstrap.inc
   @@ -1405,11 +1396,11 @@ function drupal_bootstrap($phase = NULL) {
              require_once __DIR__ . '/common.inc';
   -          _drupal_bootstrap_code();
   +          DrupalKernel::bootCode(Request::createFromGlobals());
   

   shouldnt we remove the require_once too?
   kernel will take care loading it

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -43,6 +50,91 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +  /**
   +   * Fourth bootstrap phase: initialize database layer.
   +   */
   +  const BOOTSTRAP_DATABASE = 3;
   

   unneeded too?

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -43,6 +50,91 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +  /**
   +   * Fifth bootstrap phase: initialize the variable system.
   +   */
   +  const BOOTSTRAP_VARIABLES = 4;
   

   this should be removed

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -182,32 +343,526 @@ public function boot() {
   +    // @todo Remove this if not needed.
   +    // require_once DRUPAL_ROOT . '/core/lib/Drupal.php';
   

   ok

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -182,32 +343,526 @@ public function boot() {
   +   * Finish booting by loading remaining includes and enabled modules.
   

   Finishes

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -182,32 +343,526 @@ public function boot() {
   +   * Attempt to serve the page from cache.
   

   Attempts

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -182,32 +343,526 @@ public function boot() {
   +    // @todo Use the current_user proxy.
   

   Can we try using it?

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -182,32 +343,526 @@ public function boot() {
   +      // @todo Use the services from the container.
   

   Is it still pending or this is for follow up?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -182,32 +343,526 @@ public function boot() {
+    // Enter the request scope so that current_user service is available for
+    // locale/translation sake.
+    static::$singleton->container->enterScope('request');
+    static::$singleton->container->set('request', $request);
...
+    if ($this->container->isScopeActive('request')) {
+      // Back out scope required to initialize the file stream wrappers.
+      $this->container->leaveScope('request');
+    }

needs @todo remove once https://drupal.org/node/2028109 is in

Since this is a very important patch, I started to help @larowlan to get it done behind the scenes. We're collaborating with concurrent branches in the Platform sandbox.

My changes of yesterday, which should address all of the nitpicks from earlier comments + many additional ones:

1. Fixed some code style issues + reverted unnecessary/unrelated changes.
2. Relocated DrupalKernel::boot() to match HEAD + retain more sensible place.
3. Fixed code style of ThemeNegotiatorRequestSubscriber service definition.
4. Fixed #122.1: common.inc must be included by DrupalKernel::bootCode().
5. Fixed bootCode() unnecessarily traverses directories for configurable includes.
6. Synced bootCode() with latest _drupal_bootstrap_code().
7. Removed stale _drupal_bootstrap_code() + replaced all references to it.
8. Removed obsolete BOOTSTRAP_DATABASE → #2159501: Remove DRUPAL_BOOTSTRAP_DATABASE
9. Removed obsolete BOOTSTRAP_VARIABLES → #2167105: Remove DRUPAL_BOOTSTRAP_VARIABLES
10. Fixed #123.1: Removed stale inclusion of Drupal.php.
11. Fixed other nitpicks from #123.
12. Relocated DrupalKernel::handle() to match HEAD.
13. Removed unnecessary BOOTSTRAP_COLD constant.
14. Relocated confPath() + requestPath() to match HEAD + stop distorting bootstrap phase methods.
15. Relocated bootEnvironment() + bootSettings() + bootPageCache() to match HEAD + bootstrap phase order.

@larowlan and I already discussed some further changes that will make it simpler to review this patch, so stay tuned.

If time permits, I also want to try whether it is possible to generate a diff for the moved code between bootstrap.inc and DrupalKernel to further facilitate reviews.

Major update. Simplified changelog of changes performed by @larowlan + me:

1. Rename ::bootCode to ::bootKernel
2. Rename ::finishBoot to ::bootCode
3. Reverted unrelated/out-of-scope changes to DRUPAL_ROOT.
4. Actually *use* Symfony Request in bootEnvironment().
5. Fixed settings + request initialization are not bootstrap phases, but utility functions.
6. Synced stale confPath() + bootConfiguration() + initializeSettings() + initializeRequest() with HEAD.
7. Converted all code to use DrupalKernel::boot*() instead of drupal_bootstrap().
8. Added DrupalKernel::isBootLevelReached() + converted all uses of drupal_get_bootstrap_phase().
9. Removed legacy phase logic from drupal_bootstrap() + various clean-ups.
10. Replaced DrupalKernel::createFromRequest() with ::bootKernel() for consistency (for now).
11. Make tests properly re-bootstrap DrupalKernel.

Also attaching an interdiff, but since we had to merge latest 8.x a few times due to conflicts, the interdiff is a patch-serial reflecting the commit history (sans merges).

I know it is very basic functionality and if it is not working right then Drupal will not work but I have to ask is there a follow up issue to add unittest for DrupalKernel?

Merged 8.x + fixed merge conflicts after #2228341: Objectify session management functions + remove session.inc

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -43,6 +50,70 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
+  /**
+   * Global DrupalKernel singleton.
+   *
+   * @var self
+   */
+  protected static $singleton = FALSE;

Is this scheduled to go away after everything's all injected? (Please say yes.)

It makes a bunch of the kernel untestable.

I raised similar concerns in IRC. Crell pointed out that most of the global environment stuff could be called multiple times without problem. Someone else pointed out we could have a factory that did the environment cleaning.

There are some other things like Settings that make a singleton sort of required ATM though... If you instantiate 2 kernels with different requests built from different paths, you could replace and completely destroy the setting state of the first Kernel.

If you instantiate 2 kernels with different requests built from different paths, you could replace and completely destroy the setting state of the first Kernel.

You mean like SimpleTest will be doing? :-)

The kernel should be available in the container, for any code that needs it, because the container is already injected everywhere.

And boom:

    $this->container->set('kernel', $this);

..there it is.

Do not use a singleton for this.

Or any singletons or static anything.

Please.

It's a design discipline that will pay off in reduced DrupalWTF.

I went through the bootstrap in the debugger and I'm quite happy with this patch. Great work! A couple thoughts which can be deferred to follow-up issues if desired.

1. common.inc is included quite early in bootKernel(). Thats unlike all prior versions of Drupal. If something there needs to be loaded early, it should be moved to bootstrap.inc IMO
2. The page cache check is kind of a big deal and it feels "hidden" within createKernel(). Maybe make it "visible" in index.php? Not a big deal.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -159,12 +228,30 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +    if ($class_loader) {
   +      $this->classLoader = $class_loader;
   +    }
   +    else {
   +      $this->classLoader = drupal_classloader();
   +    }
   

   Why does this parameter need to be optional? Is this for testing?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -200,7 +300,637 @@ public function getContainer() {
   +    if (isset($request)) {
   +      $request = $request;
   +    }
   +    // @todo This case cannot be possible. Remove?
   +    elseif (\Drupal::hasRequest()) {
   +      $request = \Drupal::request();
   +    }
   +    // @todo Remove once external CLI scripts (Drush) are updated.
   +    else {
   +      $request = Request::createFromGlobals();
   +    }
   

   The first case is a no-op; the second is documented as impossible. So just reduce this to the 3rd option.

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -200,7 +300,637 @@ public function getContainer() {
   +    static::$confPath = find_conf_path($http_host, $script_name, $require_settings);
   

   Can't we turn find_conf_path() into something less global-function-y? We're moving everything else around, why not this?

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -200,7 +300,637 @@ public function getContainer() {
   +      install_goto('core/install.php');
   

   Is install_goto() small enough that we can just inline it and avoid the function call? (DRY should not be taken to the point that it forces tight coupling.)

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -200,7 +300,637 @@ public function getContainer() {
   +    // Extra slashes can appear in URLs or under some conditions, added by
   +    // Apache so normalize.
   

   The sentence grammar is poor here. I think you mean: "Extra slashes may appear in URLs in some circumstances. For instance, they may be added by Apache. Normalize the path to never have a leading or trailing slash.

6. +++ b/core/modules/system/tests/http.php
   @@ -5,19 +5,27 @@
   +$kernel = DrupalKernel::bootKernel($request);
   +$kernel->setTestOnly(TRUE);
   

   Shouldn't we therefore pass the test-only flag in through bootKernel()?

Unsurprisingly, I am very uncomfortable with all of the static methods here. Most seem like they should just be folded into the kernel object's initialization process (either in handle() or boot() or whatever). I understand that we still have enough legacy code that it's hard to fix here, though. Is there a plan moving forward to deprecate and remove those? If so, they should be marked as such.

As Miles said, Singletons are teh evilz.

Moshe: common.inc vs. bootstrap.inc was original, AFAIK, to minimize the amount of code that needed to be parsed for a page cache hit. However, when using an opcode cache there is no code parsing so that distinction is no longer meaningful, and I'm still pushing for page caching to be handled separately anyway. (And it's already handled by a reverse proxy in high-performance cases anyway.) In practice I'd say that distinction is vestigial, and we only even have it for legacy reasons. At some point we should probably just combine those files, and/or refactor them out of existence.

Thanks Crell. Some good points, I'll look into them in my refactoring.

1) I think so... We like our shortcuts. I'll play with making it required.

2) So, I killed the no-op. The comment on the second case is actually wrong though, we can actually hit that code. There are lots of calls to conf_path() sprinkled around in hook_install's and such. The longer term solution would be to make it required so the caller gets explicitly clear behavior but I didn't go that far.

3) I don't see why not. I don't see any explicit calls in core so it seems like internal implementation.

4) err... um... sure? Tecnhically this is the functions documented reason for existance though and it is ugly.

5) no problem

6) sure

So on the statics(and singleton), I've done most of the refactor removing the singleton but sun raised some valid concerns so I'm re-refactoring. Not prime time ready yet but hopefully will get something together this week.

Re:common.inc vs bootstrap.inc, I actually rolled some of that out already. Not so much because of loading less code, but because really common.inc is part of the "boot code" step anyways. And with an explicit page caching step like moshe mentioned this just naturally moves back and works cleanly.

Jumping back I liked moshe's explicit page cache step and incorporated it. It let frontends not doing page caching explicitly bypass caching(win), it cleaned up some responsibilities(win) and some code and it opens up a possible API for returning a request rather then outputing and exiting(win). On miss it just returns $this. Currently it outputs and exists still but it could return a simpel HttpKernelInterface that does no routing and just returns the cached response instead of returning a response directly from the method but it would still be fairly clean.

Something like this:

public function handleCache($request) {
  if (/*cache hit*/) {
    // build response.
    return new StaticKernel($response);
  }
  return $this;
}

To get rid of the singleton and factory entirely I had to do a few more changes then I wanted. With all the merges I had to do, I don't have a useful interdiff. sorry.

The test bot can't seem to handle this patch but I wanted to get it over here for review. The testing issue I ran it on the runner failed in weird ways installing the parent site. The latest version of Drush has no problem installing the site but testbot is pinned to a commit in December and I can't get that to even work locally. So basically, this is blocked on #2206501: Remove dependency on Drush from test reviews or updating drush in the pifr puppet repo.

Until we get #2206501: Remove dependency on Drush from test reviews, rfay offerred to up the SHA1 on the drush checkout. I'm willing to make a 7.0.0-unstable1 tag for the benefit of folks who need to deploy drush7 now, like testbot, acquia, pantheon, etc.

lets see what the bot says. minor doc changes and some merges.

Finally green. No interdiff because all the merge conflict resolutions from several back merges to keep this in sync made it pretty useless.

That said, its actually mostly the same patch because the code worked. I just had to do a lot of hacking of the hard coded psuedo bootstraps in simpletest to use the kernel instead. There is one change to the kernel bootstrap though to support this (I know, yuck). I moved manually settings up the container from DrupalKernel::boot() into a lazy load in DrupalKernel::getContainer(). The reasoning is a bit complicated but it boils down to boot() initializing/resetting global Settings and tests needing to change settings then reboot the kernel/container to run a different set of tests. Simpletest therefore has to change the global settings after boot and before the container is built.

Still a great patch :). I have only minor comments

+ // Immediately boot a new kernel into the regular production environment.

This could be confused with a site’s prod environment. Can we disambiguate a bit?

+    // Start a page timer:
+    Timer::start('page');

can this move earlier in the request, like right after autoloading is available?

+  // Load legacy include files.
+  foreach (glob(DRUPAL_ROOT . '/core/includes/*.inc') as $include) {
+    require_once $include;
+  }

This bit, from run-tests.sh, is a bit surprising. I didn't look into why we need to do this manually here. Care to explain?

+  $response = $kernel
+    ->handlePageCache($request)
+    ->handle($request)
+      ->prepare($request)->send();

The indentation of the last line is two more spaces than expected. Is that deliberate?

Good questions.
The installer is hard to follow but I think at this point booting to the "production" environment is sort of what is happening. That said, maybe this is more clear:

// Use the kernel to boot into a full Drupal environment ton continue installing modules.

I think I just kept the location of Timer from previous patches which follows the flow of the old bootstrap. I don't see why we can't move it earlier though.

Yeah, this one is complicated. I did actually try removing it but this code can't call bootCode() because it is setup to run a kernel without a settings file and the container will run in to trouble. If I remember correctly, the problem is the lack of 'hash_salt' and calls to module_handler in bootCode() that setup the file stream wrappers.

Wasn't sure if anyone would notice that. Actually I did the indentation because handlePageCache() and handle() are being called on kernel's while the prepare() and send() get called on a response object.

Cleaned up the documentation in the installer. Moved Timer. Documented the response handling.

Actually, it looks like the getContainer() fix I added might let us get rid of that legacy include code in run-test. Ran some spot tests but lets see what the testbots say.

err, test bots.

142: kernel-bootstrap-142.patch queued for re-testing.

Merge commit is the work I did to merge #2248985: ScriptTest fails on Windows, runs against parent site #2240007: Regression: early installer is not in RTL after selecting RTL language #2176621: Remove global $databases and #2225605: Use request stack in form builder. Mostly just shifting things around but I did add request stack setup to preHandle to support the "bootstrap" changes made in #2225605: Use request stack in form builder.

There was a warning I managed to trigger that exposed that at some point we missed adding the testOnly variable to the DrupalKernel class so that's in the interdiff.

After plenty of discussions between @neclimdul and me in IRC over the past few weeks, in which he suggested many debatable changes, I was very hesitant to look at the refactored patch, because I feared tons of changes I'd disagree with... ;)

But turns out that this re-incarnation looks very sane now. @neclimdul successfully resolved almost all of my concerns. :-)

tl;dr: The current patch is a definitive improvement over the last incarnations of @larowlan and me. It gets rid of the singleton and cleanly resolves some other aspects. We're 95% done, only a few issues remain, ~3 major, the rest is minor (but should be addressed nevertheless). I expect this patch to be RTBC and committable in the next 1-3 days.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
      public function getContainer() {
   +    // Container should probably always be built during boot() but because of
   +    // the way Settings are loaded we need to delay it to facilitate
   +    // modification for testing. globals --
   +    if (!isset($this->container)) {
   +      $this->initializeContainer();
   +    }
   +    if ($this->containerNeedsDumping && !$this->dumpDrupalContainer($this->container, static::CONTAINER_BASE_CLASS)) {
   +      watchdog('DrupalKernel', 'Container cannot be written to disk');
   +    }
        return $this->container;
      }
   ...
   +  public function bootCode(Request $request) {
   ...
   +    // Ensure container is loaded.
   +    $this->getContainer();
   

   Hm. This looks very fragile to me. I expect boot() to boot up a service container, regardless of what may or may not be called afterwards.

   Is it possible that the change to move the container initialization into getContainer() is a stale remnant of your former refactoring attempts?

   I can't really see a reason for why the container initialization couldn't be moved back to the end of boot() now.

2. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   +  /**
   +   * Prepare the kernel for handling a request without handling the request.
   +   *
   +   * Because Drupal still provides so much outside of the Kernel as global state,
   +   * there are standalone php files even within core that want to handle the page
   +   * request entirely on their own but want to have access to this state. To do
   +   * they can create a kernel and call this method to have the Kernel populate its
   +   * state which will be mirrored in those global methods.
   +   *
   +   * Note: This is provided for backwards compatibility only. Many of those global
   +   * methods are deprecated and the ones that are not are meant to be shortcuts for
   +   * procedural methods, not for bypassing the kernel. Future code should extend the
   +   * DrupalKernel or implement its own kernel and handle the page the request in
   +   * that class.
   +   *
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The current request.
   +   */
   +  public function preHandle(Request $request);
   

   Let's rename this method to prepareLegacyRequest() to be 100% clear.

   preHandle() sounds like something I'm supposed to call in a modern front-controller.

   prepareLegacyRequest() is something I won't bother to even look at. :-)

   Let's also shorten the phpDoc... it's veeery wordy/slangy. ;)

3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      protected function add($type, $name, $path) {
        $pathname = "$path/$name.info.yml";
   +    if (!file_exists(DRUPAL_ROOT . '/' . $pathname)) {
   +      throw new \InvalidArgumentException("Unknown $type: $pathname not found.");
   +    }
   
   +++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityFormDisplayTest.php
      public function testDeleteFieldInstance() {
   -    $this->enableModules(array('field_sql_storage', 'field_test'));
   +    $this->enableModules(array('field_test'));
   
   +++ b/core/modules/field/lib/Drupal/field/Tests/Email/EmailItemTest.php
   -  public static $modules = array('email');
   +  public static $modules = array('text');
   
   +++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/EmailFieldRdfaTest.php
   -  public static $modules = array('email', 'text');
   +  public static $modules = array('text');
   

   Hm. These changes seem to have been separated into/incorporated from #2246611: Document ModuleHandler pre-conditions

   Let's get that patch in first.

4. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
   @@ -218,6 +224,7 @@ protected function tearDown() {
      public function containerBuild(ContainerBuilder $container) {
   +
        // Keep the container object around for tests.
   

   Note: (just because this pattern disturbingly repeats itself through the entire patch)

   We do not add a blank line at the top of functions and methods.

   It would be good to remove all of those superfluous lines throughout the touched/added functions in this patch... Most seem to have an extraneous blank line at the beginning.

   Otherwise, someone will have to clean up after you ;)

   (Also happy to do that + some more comment polishing, once the latest/final changes are pushed into the sandbox.)

5. +++ b/core/modules/simpletest/lib/Drupal/simpletest/InstallerTestBase.php
   @@ -109,7 +110,7 @@ protected function setUp() {
        // Import new settings.php written by the installer.
   -    drupal_settings_initialize();
   +    Settings::initialize($this->container->get('request'));
   

   We're removing the minimal/mocked container from TestBase in #2196241: Remove string translation services from TestBase container, so $this->container->get('request') will no longer be available.

   Let's just replace that with Request::create('/install.php');

6. +++ b/core/rebuild.php
   +$kernel = new DrupalKernel('prod', $autoloader);
   +$response = $kernel->preHandle($request);
   ...
   -  drupal_rebuild();
   +  drupal_rebuild($kernel, $request);
   

   Already mentioned + discussed in IRC:

   Unfortunately, this breaks the purpose of rebuild.php / drupal_rebuild().

   rebuild.php is run when it's not possible to boot a kernel (due to changed code), so the change to boot a kernel upfront and inject it into drupal_rebuild() doesn't work.

   rebuild.php only boots to _CONFIG in HEAD, so as to initialize Settings. drupal_rebuild() then proceeds to kill each and everything + rebuild all stuff from scratch.

   We can and should revisit this code, but for now, let's simply restore the behavior of HEAD, which should be easily possible by calling Settings::confPath() and Settings::initialize() in sequence.

7. +++ b/core/tests/bootstrap.php
   +define('DRUPAL_ROOT', realpath(__DIR__ . '/../../'));
   

   AFAIK, a lot of people will disagree with this.

   I looked through the patch but wasn't able to see why we suddenly have to tamper all PHPUnit test environments now.

   Also note that I had removed all off-topic/out-of-scope changes regarding DRUPAL_ROOT from this patch in an earlier iteration already...

8. +    ->handle($request)
   +      ->prepare($request)->send();
   

   Minor: Let's simply vertically align each of these chained calls below each other (like we do everywhere else) ;-)

simple answer, simpletest.

longer answer, it grep more complex, gained a singleton, got renamed to bootKernle() then backlash removed it entirely. I'm going to take a stab at bringing it back and seeing if it can take request out of boot. We'll see how far it gets.

There shouldn't be a 'boot' method.

There should be a kernel object, and you tell it to do the thing you want to do, like this: https://github.com/symfony/symfony-standard/blob/master/web/app.php#L26-L29

Similarly, there shouldn't be a factory method that creates a kernel based on a request. That's silly. There's only one kind of kernel you can ever create, which is why it's the kernel.

Also, static methods::bad, testable methods->good.

Ok, createFromRequest() factory method, documentation changes from sun's pull. Also, from feedback from Mark I moved the conf_path replacement to DrupalKernel::sitePath(). Name change my design though because its more accurate. Move because Settings is a value object and it was actually that way in earlier patches.

The only thing I think I didn't do was sun's point #8 about indentation. There's documentation attached to it and the other places we indent on the same line we do because we're chainging $this returns but this line changes to a request object. The indentation calls out the object change.

I read through this *again* and its still awesome.

Thanks for the countless rerolls, @neclimdul.

Let's try to cut down on the endless re-rolls.

Conflicted with #2228215: Remove module check in DrupalKernel :(
Which to add insult to injury, was committed, reverted, and committed despite this being tagged "avoid commit conflicts".

EDIT: Also conflicted with #2259275: Rename "DrupalUnitTestBase" to KernelTestBase, but AFAIK that was just a rename.

This is an attempt at a merge with 8.x, but I'm not familiar with the patch so may have resolved some conflicts incorrectly. Feel free to ignore and merge from scratch if this isn't useful.

yeah... figured it might come back red. Thanks for trying though! Those where going to be another tricky merges but I'm working on it.

Related idea:
#2272129: [meta] Introduce a low-level DIC to organize low-level services

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -174,9 +244,120 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
+
+  public function setSitePath($path) {
+    $this->sitePath = $path;
   }

Missing doc blocks and I'm not sure of the point of this method. Yes it is called once but this is the onlt place $this->sitePath is set or accessed. I think it can be removed and the call to it. But what this means is that we've lost the static caching of the procedural conf_path() method. I don't know the performance impact of this because I don't know the amount of times this needs to be accessed per request.

Think this gets all the fixes. There are some subtle changes to WebTestBase, mostly that it has to be able to dump the container to disk during rebuildContainer() now because the request won't automatically fix the container.

Good catch in a complicated patch alex. I though I should just kill it then looked at the number of calls to conf_path() and went ahead and documented it and added a get method. Also documented the deprecation noticed on conf_path(). I figure the individual conversions will know whether there's a kernel available or if it needs to build the path directly from a request like in the installer and choose the correct method. these changes are in the not-really-an-interdiff interdiff.

In my mind i changed the status. d.o disagrees though.

unmerge merge with #2239969: Session of (UI) test runner leaks into web tests. revert cause patch apply to fail.

Are there any new classes that might get autoloaded from this before the APC classloader can be initialized?

new Patch.

catch, that came up at one point but I don't have the list. I'll come back shortly with the numbers but want to get testbot running.

echo $class . "\n"; on first line of composers classloader. Not many surprises here. Classes and interfaces are those related to the kernel with the exception of Timer which was moved earlier at moshe's request and Unicode who's ::check() call I moved to the environment initialization for clarity.

Head:

Drupal\Core\Database\Database
Drupal\Core\Site\Settings

Patch:

Symfony\Component\HttpFoundation\Request
Symfony\Component\HttpFoundation\ParameterBag
Symfony\Component\HttpFoundation\FileBag
Symfony\Component\HttpFoundation\ServerBag
Symfony\Component\HttpFoundation\HeaderBag
Drupal\Core\DrupalKernel
Drupal\Core\DrupalKernelInterface
Symfony\Component\HttpKernel\HttpKernelInterface
Symfony\Component\HttpKernel\TerminableInterface
Drupal\Component\Utility\UnicodeDRUPAL_BOOTSTRAP_CONFIGURATION
Drupal\Core\Site\Settings
Drupal\Core\Database\Database
Drupal\Component\Utility\Timer

Merge namespace conflict. Alex and Catch, any thoughts?

Overall this looks great!
Most of those below are nitpicks, couldnt find something wrong..i am not very sure about the introduction of Settings::initialize() but we can revisit later if needed

1. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -54,6 +57,22 @@ public function getServiceProviders($origin);
   +   * Set the current site path.
   +   *
   +   * @param $path
   +   *   The current site path.
   +   */
   +  public function setSitePath($path);
   

   would be nice to return $this here.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ThemeNegotiatorRequestSubscriber.php
   @@ -0,0 +1,65 @@
   +  public function __construct(ThemeNegotiatorInterface $theme_negotiator) {
   +    $this->themeNegotiator = $theme_negotiator;
   +  }
   

   seems the negotiator is unused. remove?

3. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -7,6 +7,10 @@
   +use Drupal\Core\DrupalKernel;
   +use Symfony\Component\HttpFoundation\Request;
   

   unneeded?

4. +++ b/core/lib/Drupal/Core/Test/TestRunnerKernel.php
   @@ -11,20 +11,23 @@
   +  public static function createFromRequest(Request $request, ClassLoader $class_loader, $environment = 'test_runner', $allow_dumping = true) {
   +    return parent::createFromRequest($request, $class_loader, $environment);
   +  }
   

   Needs inheritdoc and strtoupper on true

5. +++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorLoadingTest.php
   @@ -129,12 +131,14 @@ function testLoading() {
   -    $expected = array('formats' => array('filtered_html' => array(
   -      'format' => 'filtered_html',
   -      'editor' => 'ckeditor',
   -      'editorSettings' => $ckeditor_plugin->getJSSettings($editor),
   -      'editorSupportsContentFiltering' => TRUE,
   -      'isXssSafe' => FALSE,
   +    $expected = array(
   +      'formats' => array(
   +        'filtered_html' => array(
   +          'format' => 'filtered_html',
   +          'editor' => 'ckeditor',
   +          'editorSettings' => $ckeditor_plugin->getJSSettings($editor),
   +          'editorSupportsContentFiltering' => TRUE,
   +          'isXssSafe' => FALSE,
   

   irrelevant?

6. +++ b/core/modules/system/tests/http.php
   @@ -5,19 +5,29 @@
   +    // Handle the response object.
   +    ->prepare($request)->send();
   
   +++ b/core/modules/system/tests/https.php
   @@ -8,18 +8,28 @@
   +    // Handle the response object.
   +    ->prepare($request)->send();
   
   +++ b/index.php
   @@ -8,13 +8,22 @@
   +      // Handle the response object.
   +      ->prepare($request)->send();
   

   Unneeded indentation. also maybe have send() in separate line as well.

7. +++ b/core/rebuild.php
   @@ -11,25 +11,30 @@
   +header('Location: ' . $base_path);
   

   why not a RedirectResponse?

8. +++ b/core/update.php
   @@ -297,19 +297,23 @@ function update_task_list($active = NULL) {
    require_once __DIR__ . '/includes/common.inc';
    require_once __DIR__ . '/includes/file.inc';
    require_once __DIR__ . '/includes/unicode.inc';
   ...
    require_once __DIR__ . '/includes/schema.inc';
   ...
    require_once __DIR__ . '/includes/database.inc';
   ...
   +$kernel->boot();
   

   Lets also remove more require statements here:) there will be loaded in boot()

1) I'm not sure why. That doesn't seem to be something we do on setters.
2) fixed.
3) yeah, missed those. fixed.
4) fixed.
5) Came up before. As is standard with chaining, indentation is used to show the change in the object being interacted with. This is also in the documentation to make it even more clear.
6) That's actually the way it already is in core and we're in such a limited environment in that script and there isn't any code to interact with the response so I didn't see a reason to change it and add the complexity.
7) Changing update.php makes me really nervous. Done, lets hope it comes back green. :)

Nice work! Back to RTBC