Problem/Motivation

While working on the Fault system I've become somewhat familiar with the current organization of DrupalKernel and find it to be somewhat haphazard. My main concern is the class has five distinct tasks

  1. Setting the Environment
  2. Loading Legacy Support Files
  3. Initializing the Service Container
  4. Building/Rebuilding the Service Container
  5. Script Shutdown

Attached is a diff file proposing a solution to at least one part of this problem - the set up of the environment. Currently this is the part of Drupal Kernel that's in the worst shape for these reasons

  1. The point of entry into the class is ambiguous (too many public methods)
  2. The class spends considerable effort trying to track its own boot state.
  3. There are conditionals in the code (and I imagine throughout Drupal) that change behavior between test mode and normal mode. While I understand that this is unavoidable to some extent, a better job can be done separating test code from production. Ideally no test code should be loaded during production.

We can do better and need to for these reasons if for no other reason than making debugging life easier.

Proposed resolution

The attached patch shows one possible solution - place the Environment setup in their own family of classes. The setup of the environment is done as part of the kernel's construction. Index.php will return to a pruned version very, very similar to the one in issue 2389811 --


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

$autoloader = require_once 'autoload.php';

$request = Request::createFromGlobals();

$kernel = new DrupalKernel($request, 'Production', $autoloader);

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

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

The only real difference is the request is created before the kernel so that it can be fed into the kernel as a start argument. DrupalKernel then decides which Environment to load based on the keyword passed in. This is the logic for determining the class to load.

  /**
   * Boots an environment and binds it..
   */
  protected function setEnvironment($request, $environment, $class_loader) {
    // This check for simpletest requests isn't thorough, it's just enough to
    // load the correct test profile which in turn will verify.
    if (isset($_SERVER['HTTP_USER_AGENT']) && stristr($_SERVER['HTTP_USER_AGENT'], 'simpletest')) {
      $environment .= 'Test';
    }
    $class = '\\Drupal\\Core\\Environment\\' . $environment . 'Environment';
    $this->environment = new $class($request, $class_loader);
  }

The above check of the user agent is the only remaining reference to the existence of simpletest existing outside of the ProductionTestEnvironment class, which is built to test the ProductionEnvironment.

Each of the Environments extend off an abstract base environment. Here is its constructor, which is about as broken down as possible.

    /**
     * Return an Environment object.
     */
    public function __construct(Request $request, $class_loader) {

      // The very basics - who, where when.
      $this->defineProfile();
      $this->defineRoot();
      $this->defineRequestTime();

      // Fault Management
      $this->setAssertHandler();
      $this->setDeprecationErrorHandler();
      $this->setStrictAndNoticeErrorHandler();
      $this->setPrimaryErrorHandler();
      $this->setExceptionHandler();

      // Normally we'd do this assertion at the start, but we needed to set
      // The handlers first in case we need to handle a raised fail.
      assert('\\Drupal\\Component\\Fault\\Assertion::validCaller(\'\\Drupal\\Core\\DrupalKernel\')',
        'Only DrupalKernel should use this object.'
      );

      // Now the last of the low level settings.
      $this->setEnvironment();
      $this->setStringHandling();
      $this->defineTestState();

      // Attach the request and loader.
      $this->setRequest($request);
      $this->setLoader($class_loader);

      // Determine the site.
      $this->findSitePath();
      $this->loadSettings();

      // Verify the Host.
      $this->setHostPatterns();
      $this->setupTrustedHosts();

      // Get the Database ready.
      $this->setupDatabase();

      // And now we begin timing the page.  If we need to check the efficiency
      // of the forgoing process we can check against the
      // $_SERVER['REQUEST_TIME'].
      $this->startPageTimer();

    }

Note that bootstrap.inc is *gone*. In it's place is the Environment files themselves, taking advantage of the rarely used multi-namespace trick - you can see this in the diff but the basic structure of the files is

namespace {
  // Environment's global scope constants and functions
}

namespace Drupal\Core\Environment {
  // Class file is placed here.
}

This way only the functions that are going to be used in a given scenario will be loaded.

The huge advantage of this approach is that only the functions we intend to use in a given environment scenario get loaded. Even better, the different scenarios can, in theory, have different versions of the same function, so long as neither function file gets loaded at the same time. In this test patch I only did this once - giving production a faux version of drupal_valid_test_ua() that always returns null with no other logic for backwards compatibility.

As can be seen from the construct function above, the child classes have a high degree of control over the process through overrides. For example, I would imagine the installer environment would have an empty setDatabase method, at least for the installer phases before the database is ready.

The environments being classed this way also puts a reason not to allow an arbitrary environment argument.

Anyway, the patch was tested to work on an already installed site, and just on the home page. My intent was to get one working page out so that I have enough working code to demonstrate the concept. This approach has the massive downside of shaking the API up some, though by how much I'm not sure, but certainly enough to make this an 8.1 item or even later. I'm just putting this out there for the moment with the question - is this a direction we want to go in?

Remaining tasks

Debate on this general course of action and deciding the scope and degree of change that will be acceptable within the 8.1 branch.

User interface changes

None anticipated - this is an issue of the organization of Kernel.

API changes

As noted above, to be decided. This is currently and exploratory proposal. Hopefully none -- I'd prefer this to entirely be an internal API shift up. However since the internal API isn't completely defined (as distinct from the module facing API) every single public method in DrupalKernel that is removed has the potential to affect a module that called it.

Comments

znerol’s picture

The only real difference is the request is created before the kernel so that it can be fed into the kernel as a start argument.

This is exactly what #2389811: Move all the logic out of index.php (again) tries to fix. It is very weird and unnecessary that currently kernel construction depends on the request.

Also this issue looks similar to #2282779: [meta] DrupalKernel has too many responsibilities. A agree with the problem statement, but I think that instead of moving around static methods again (#2016629: Refactor bootstrap to better utilize the kernel already did that once), we should extract the weird parts one at a time from the kernel (and bootstrap.inc) into dedicated services.

Aki Tendo’s picture

This is exactly what #2389811: Move all the logic out of index.php (again) tries to fix. It is very weird and unnecessary that currently kernel construction depends on the request.

The only reason it's necessary at the moment is the kernel needs to know if it is in test mode right off the bat. That is currently clued off the HTTP_USER_AGENT header. It would be better to do this off a custom HTTP header and let Apache do the routing to a test front script separate from index.php. That is also the only way the kernel could be started without the request I can see.

znerol’s picture

The only reason it's necessary at the moment is the kernel needs to know if it is in test mode right off the bat.

It does not need to know that during construction. It is enough when checking that before the request is handed off to the http kernel handle() method (for normal/non-legacy requests).

Aki Tendo’s picture

Disagree. If it doesn't know if it's in test mode it's forced to load all the test monitoring code "just in case" - an unnecessary overhead for production.

Also, booting the kernel means getting ready for *any* request, normal or legacy. This scattered approach is why the kernel has what, 5 starting functions and has to repeatedly check to see if boot has been called yet instead of following a sane systematic approach to set up.

Aki Tendo’s picture

Status: Active » Needs review
StatusFileSize
new48.55 KB
new4.95 KB

Ok, following several IRC conversations and the above I'm going to start baby-stepping through this. First patch to test - separating Simpletest and productions entry scripts. The attached patch uses htaccess to force all simpletest requests to hit an index.php that lives in the simpletest module instead of production's. The goal of this is to relieve DrupalKernel of the task of determining if it is in production mode or test mode.

Note, this is built on top of the FaultSystem patch - see the interdiff for what is unique to this patch.

Status: Needs review » Needs work

The last submitted patch, 5: patch-2465447-1.diff, failed testing.

Aki Tendo’s picture

Status: Needs work » Needs review
StatusFileSize
new3.18 KB
new46.79 KB

Oops, left a test command in there.. Let's try this again.

Status: Needs review » Needs work

The last submitted patch, 7: patch-2465447-2.diff, failed testing.

Aki Tendo’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review

Aki Tendo queued 7: patch-2465447-2.diff for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: patch-2465447-2.diff, failed testing.

martin107’s picture

When I run this locally FeedParserTest passes ... it fails on testbot.

Was that the reason for #10 and #11?

Aki Tendo’s picture

Issue was originally marked as 8.1.x so the testbot was trying to bind the patch to 8.1, whereas the patch was built against 8.0.x so I had to change that out at #9

martin107’s picture

Status: Needs work » Needs review

Ok lots has changed in the last 24 hours - which is good thing.

So I still want to if there is still a difference between testbot and me.

Here are the stats before I retest

55,722 pass(es), 21,216 fail(s), and 1,644 exception(s).

triggering testbot.

martin107 queued 7: patch-2465447-2.diff for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: patch-2465447-2.diff, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +stale-issue-cleanup

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

smustgrave’s picture

Status: Postponed (maintainer needs more info) » Closed (outdated)

This seems very similar to #2282779: [meta] DrupalKernel has too many responsibilities which is new.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.