Inject DRUPAL_ROOT into DrupalKernel

Le patch.

Ah, if only I could get the local testbot working....

Three's a charm?

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -252,15 +252,18 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +   * @param string $app_root
   +   *   (optional) The path to the application root as a string. If omitted or
   +   *   NULL, the application root will be computed.
   
   @@ -277,12 +280,28 @@ public static function createFromRequest(Request $request, $class_loader, $envir
   +   * @param string $app_root
   +   *   (optional) The path to the application root as a string. If omitted or
   +   *   NULL, the application root will be computed.
   ...
   +    if ($app_root === NULL) {
   
   @@ -324,6 +343,9 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE)
   +   * @param string $app_root
   +   *   (optional) The path to the application root as a string. If omitted or
   +   *   NULL, the application root will be computed.
   

   See the === ?

   It means we need to document string|null, because the type of the parameter matters to the code.

2. +++ b/core/lib/Drupal/Core/Test/TestRunnerKernel.php
   @@ -20,15 +20,15 @@ class TestRunnerKernel extends DrupalKernel {
   -  public static function createFromRequest(Request $request, $class_loader, $environment = 'test_runner', $allow_dumping = TRUE) {
   -    return parent::createFromRequest($request, $class_loader, $environment);
   +  public static function createFromRequest(Request $request, $class_loader, $environment = 'test_runner', $allow_dumping = TRUE, $app_root = NULL) {
   +    return parent::createFromRequest($request, $class_loader, $environment, $allow_dumping, $app_root);
   

   I'm pretty sure the reason we override createFromRequest is so we can insert our new defaults for optional parameters. Otherwise we don't really need it. So yay, that stays. But....

3. +++ b/core/lib/Drupal/Core/Test/TestRunnerKernel.php
   @@ -20,15 +20,15 @@ class TestRunnerKernel extends DrupalKernel {
   -  public function __construct($environment, $class_loader) {
   -    parent::__construct($environment, $class_loader, FALSE);
   +  public function __construct($environment, $class_loader, $allow_dumping = FALSE, $app_root = NULL) {
   +    parent::__construct($environment, $class_loader, $allow_dumping, $app_root);
   

   On the other hand, the reason we pass FALSE to parent in the constructor is so we can explicitly always turn off dumping.

   So we do need to keep the FALSE, but also add the $app_root parameter.

Reverted the string|null in docblocks, re-added FALSE to TestRunnerKernel::__construct().

Also changed visibility of guessApplicationRoot() to be private, so that we don't end up with callers just calling it as an input to the factory, or worse using it to discover the app root.

However, what if a caller explicitly wanted to pass TRUE to the parent.

Right. Exactly. :-) The thing is that it's there to force the kernel to never allow dumping. It's the version of the kernel that runs the tests, so it shouldn't dump its container; it should always rebuild it on the next test run, and it should remain isolated from the tests.

I don't necessarily agree with it, but that's how it's designed. http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Test/TestRun...

Changing it is out of scope here. File a follow-up if you'd like.

OK, so also reverted docblocks so that now we have string instead of string|null.

we never use private functions, let's just keep using protected here

Is there a reason we never use private methods, other than we never use them?

The reason it's private is in #14.

In an ideal world that method wouldn't exist, because we'd require the injected app root rather than allowing it to be optional. Then no one would ever guess. But that would be a major refactoring and a bit of an API change. So having the guess be private discourages others from relying on the guess.

Well, the thing is, that some people maybe want to change its behaviour.

If they want to change its behavior then they can submit a patch. I made it private so people wouldn't use it outside the kernel. See #14.

So I shouldn't use private because an unknown person might think private is bad, and wouldn't be convinced by #14, and leaving it alone will waste *my* time.

Ok, phantom reviewer, this one's for you...

Here's a patch that says protected instead. If anyone other than the phantom reviews it, I'd really appreciate it if you could say something like, "Why is this method protected? It looks like it should be private, since that will encourage people to walk the path of dependency injection, an idiom we've adopted here in the Drupal project with the release of Drupal 8, and basically the raison d'etre of this patch."

Thanks.

Rerolled for 8.2.x.

We really should just force the injection of DRUPAL_ROOT without giving an option not to.