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
- Setting the Environment
- Loading Legacy Support Files
- Initializing the Service Container
- Building/Rebuilding the Service Container
- 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
- The point of entry into the class is ambiguous (too many public methods)
- The class spends considerable effort trying to track its own boot state.
- 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.