Replace most instances of the DRUPAL_ROOT constant with the app.root container parameter

@msonnabaum, @neclimdul, @Crell, and I already had some discussions on the topic of how to handle DRUPAL_ROOT, in particular for unit tests. One affected issue was #2246611: Document ModuleHandler pre-conditions

We discussed a constructor injection approach (like here), but further discussion made us prefer a getAppRoot() method that defaults to DRUPAL_ROOT but can be cleanly overridden in test stubs. Unfortunately, I don't remember what made us arrive at that conclusion, but IIRC, @msonnabaum raised some legit concerns against constructor injection (or perhaps someone else, dunno).

I think that could use some more discussion (→ plan) before investing a lot of work here, so as to ensure that the proposed solution is the desired + sustainable solution.

In any case, I don't really see the point of \Drupal::root() and would rather prefer to omit that here. I don't think we're able to remove the DRUPAL_ROOT constant anyway, since a lot of other code depends on it.

To the extent possible I fully support this. I don't think that the extent possible will be 100%, but that doesn't mean we shouldn't do the parts we can. Constructor injection for this is the ideal if we can make it work in a given context.

That said, there's probably a lot of places we're using DRUPAL_ROOT where we arguably don't even need it. That would be even better. :-)

What Crell said.

This doesn't get every instance of DRUPAL_ROOT, but it looks like it gets most of 'em. It's also still a digestable patch size. I therefore think we should go ahead with it. If we are able to convert anything else later we can do so.

I've so been there... :-)

Hmm... I actually like #18 better. I think every app should have a roo!

(Sorry, couldn't resist.)

Tried to apply patch and it failed in the following files:

core/includes/bootstrap.inc
core/includes/install.core.inc
core/lib/Drupal/Core/DrupalKernel.php
core/lib/Drupal/Core/Site/Settings.php
core/modules/simpletest/src/InstallerTestBase.php
core/modules/simpletest/src/WebTestBase.php
core/rebuild.php

Patch likely was impacted by #2325197: Remove drupal_classloader()

Not an "easy" reroll but not t0o bad. Lots off conflicts because of the addition of a classloader to the settings init which had to manually be resolved. Details in the interdiff. Not really comfortable with that change but I missed the issue so to the follow up I'll go.

Silly fast-moving core...

We've gone from 248 to 201 instances of DRUPAL_ROOT so

This doesn't get every instance of DRUPAL_ROOT, but it looks like it gets most of 'em.

And the issue title is a bit disingenuous.

I'm happy to do this replacement in several steps but let's have an honest title and followups to replace the other 200 DRUPAL_ROOT instances. I have no idea why we've converted what we've converted in this patch and why we've added additional usages of DRUPAL_ROOT

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -437,7 +437,7 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
+    $listing = new ExtensionDiscovery(DRUPAL_ROOT);

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -862,14 +862,14 @@ protected function setUp() {
+    Settings::initialize(DRUPAL_ROOT, $directory, $class_loader);
...
+    Settings::initialize(DRUPAL_ROOT, $directory, $class_loader);

For example

Retitling to be more accurate. (104 left means more than half are replaced here, which qualifies for "most".)

git ac https://www.drupal.org/files/issues/drupal_root-2328111-34.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  122k  100  122k    0     0  53564      0  0:00:02  0:00:02 --:--:-- 64681
error: patch failed: core/core.services.yml:275
error: core/core.services.yml: patch does not apply
error: patch failed: core/modules/system/src/Controller/DbUpdateController.php:61
error: core/modules/system/src/Controller/DbUpdateController.php: patch does not apply
error: patch failed: core/modules/user/src/Tests/UserAccountFormFieldsTest.php:29
error: core/modules/user/src/Tests/UserAccountFormFieldsTest.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php:167
error: core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php: patch does not apply

IMO the motivation section of the IS needs work. I don't know whats ugly about DRUPAL_ROOT. Seems pretty straightforward to me. I see it replaced with various things like $this->root, \Drupal::root(), app.root. Those aren't too bad either - just want to know whats wrong here.

moshe: Global constants are global variables. Aka, complecting with the environment. Aka the class will fatal if that's not defined, as opposed to will not instantiate without an obvious constructor parameter. That is what "ugly" means in this context.

So lets update the summary and be explicit about what we mean by ugliness then.

Defining ugly.

(Also this isn't really WSCCI, just general good architecture.)

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -355,7 +365,7 @@ public function disable(array $theme_list) {
+    if (TRUE || !isset($this->list)) {

app.root is stored on the container.

The container is stored in phpstorage. This massively changes the semantics compared to DRUPAL_ROOT which is guaranteed to be correct each request, now it can get stale.

If I copy or move my Drupal install to a different directory, what happens?

Moving - won't it fatal and require rebuild.php? (not too bad)

Copying - won't it start including files from the wrong directory? (this seems like the potential for all kinds of fun debugging situations).

thats true for d7 too..move files..you get all kind of weird stuff until you run drush rr. i dont see the problem

Interdiff please?

I was referring more to an interdiff for #20, with the runtime approach.

When I rewound head to October 10, 2014 at 8:11pm and applied the patch from #60, it failed .. I am assuming other commits about that time invalidated the reroll...

So I rerolled from #59

Not a real review but

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -108,6 +108,7 @@ class ThemeHandler implements ThemeHandlerInterface {
+<<<<<<< ours

Oops

+++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
@@ -125,8 +125,8 @@ protected function setUp() {
+    $root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));

+++ b/core/tests/Drupal/Tests/Core/Form/FormTestBase.php
@@ -160,6 +167,7 @@ protected function setUp() {
+    $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));

--- a/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskIntegrationTest.php
@@ -50,6 +57,8 @@ protected function setUp() {
+    $this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
+    $container->setParameter('app.root', $this->root);

I don't have a concrete proposal but... is there nothing more readable?

If this is absolutely necessary, can it be moved to the parent class avoiding repetition?

I know... but we want people to write unit tests :-)

I am sure people will start by copypasting core ones, and it can be difficult to realize that 99% cases need to replace

$this->root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));

with

$this->root = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__)));

if they put their modules on DRUPAL_ROOT/modules

1. +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
   +++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
   @@ -33,13 +33,22 @@ class ModuleHandlerTest extends UnitTestCase {
   
   @@ -505,6 +516,7 @@ public function dependencyProvider() {
   +    $root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
   +    $this->assertEquals(array('module' => $root . '/place'), $this->moduleHandler->getModuleDirectories());
      }
   

   This could use $this->root now I think.

2. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
   --- a/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
   +++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
   
   +++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php
   @@ -125,8 +125,8 @@ protected function setUp() {
   +    $root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
   +    $this->formCache = new FormCache($root, $this->keyValueExpirableFactory, $this->moduleHandler, $this->account, $this->csrfToken, $this->logger, $this->configFactory, $this->requestStack, $this->requestPolicy);
   

   I think here too.

3. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -46,9 +46,18 @@ class RegistryTest extends UnitTestCase {
   
   @@ -65,7 +74,8 @@ public function testGetRegistryForModule() {
   +    $root = dirname(dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))));
   +    include_once $root . '/core/modules/system/tests/modules/theme_test/theme_test.module';
   

   and here.

Thanks! Looks good to me. However I don't feel confident for RTBCing myself.

+++ b/core/includes/bootstrap.inc
@@ -284,7 +284,7 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+      $listing = new ExtensionDiscovery(DRUPAL_ROOT);

+++ b/core/includes/install.core.inc
@@ -390,7 +390,7 @@ function install_begin_request(&$install_state) {
+  $listing = new ExtensionDiscovery($container->getParameter('app.root'));

+++ b/core/includes/module.inc
@@ -179,7 +179,7 @@ function module_uninstall($module_list = array(), $uninstall_dependents = TRUE)
+  $listing = new ExtensionDiscovery(\Drupal::root());

I am confused why there are three different approaches?

Fixed up incomplete reroll.

1. +++ b/core/modules/update/src/Form/UpdateManagerInstall.php
   @@ -207,7 +207,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      $filetransfer = new Local(\drupal::root());
   

   errr... weird that it's lowercase and also this is a non-test class, shouldn't we be able to inject the dependency?

2. +++ b/core/modules/update/src/Form/UpdateReady.php
   @@ -133,7 +133,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        $filetransfer = new Local(\Drupal::root());
   

   Again, we should be able to inject this.

My only other criticism here is that ExtensionDiscovery looks like it could probably be a service in its own right. That's probably outside the scope of this patch, but it's worth noting. Overall, I really like the move away from direct dependencies on Drupal specific code, so this is generally ++ to me. Looks like just a few little issues left with it.

Eclipse

A Simple fix.

Opps .. UpdateUploadTest is now green.

I ponder if we can remove define('DRUPAL_ROOT', realpath(__DIR__ . '/../../')); from the PHPUnit bootstrap. The only test fails we have are in UnroutedUrlAssemblerTest and UrlGeneratorTest because the tests are using the constant. If we could remove the define from PHPUnit we would be even closer to removing the dependency and certainly less likely to re-introduce it anywhere unnecessarily.

That seems super sensible.

Eclipse

Back to RTBC.

Looked through this as well, and it's quite awesome.

Noticed some things, none of them should block commit, though:

1. You introduced protected $root; to UnitTestCase (which is good), but a lot of subclasses a duplicating the variable documentation. That could be omitted. (I will open a tiny, unimportant follow-up for that.)

2. +  app.root:
   +    class: SplString
   +    factory_service: 'app.root.factory'
   +    factory_method: 'get'

   As far as I can tell, this is never actually used, but instead $container->getParameter('app.root') is being used (vs. $container->get('app.root'). Am I missing something?

Awesome, thanks!

looks good.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Whilst this change is quite disruptive - we gain the benefit of code that is less coupled to global constants and the disruption is only due to patch size and not complexity so I think the benefits outweigh the risk. This patch brings us down to 107 instances of DRUPAL_ROOT (from 288). Also having to rebuild the container after moving a drupal install location seems totally reasonable to me. Committed 91c38c8 and pushed to 8.0.x. Thanks!

Can the CR be updated to include \Drupal::root()M too.