Wrong DRUPAL_ROOT with non-standard code structure

Confirming the problem, patch corrects it

but I'm not sure whether this is the correct way to handle it

oops, pasted my clipboard contents into title,
correcting xD

Hi,
I think you should at least add a comment to the patch, why you had to do this and what you do.
Another thing: why do you use '-17' as parameter? Is this constant? If so I would suggest to define one.

Nonetheless thanks for your patch :)

Please review the patch in #1540136: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc. Maybe it fixes this issue too.

I think #1540136: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc is sovling this issue too. I just tested it by using symlinks for the core files/folder in my setup and it worked fine. So i think this issue can be closed (duplicate)

With a setup as mentioned in #1 http://drupal.org/files/1540136_4.patch is not solving this issue.

leschekfm is right

@see also https://bugs.php.net/bug.php?id=46260

correct path is given by $_SERVER["SCRIPT_FILENAME"]

Note that there's also #1540136: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc, which cleans up some of this stuff. Might want to get that in before this one, since then the changes presented in #15 would just need to happen in one place rather than a few places.

#1540136: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc is in. We could probably simplify the chdir() usage here.

#15: 0001-fix-authorizeinstallupdate.phpforsymlinkedcoredirectory_1.patch queued for re-testing.

dirname(__DIR__) doesn't work?

We can't use "__DIR__" because it works with symlinks the same way as ".." would

But using dirname() is a good idea to get rid of the ugly substr() stuff!

#29: chdir_drupal_root-1792310-29.patch queued for re-testing.

beauty (readability) wins over optimization and (if actually necessary and probably not in this case) follows later.

dirname() does not need hard drive access, it's basically just stripping away anything from the end of the string until the last slash.. so it performs probably quite similar to substr()

Probably could apply the same pattern to statistics.php too.

#34: 1792310.patch queued for re-testing.

Just confirming that the patch in #34 does indeed fix issues when core files are symlinked. The code looks sane to me, but I'll wait for another pair or two of eyes before RTBC.

#34: 1792310.patch queued for re-testing.

The patch at #34 doesn't fix my problem with a symlink setup using nginx. I have a similar directory structure to the op. The problem on install is finding the settings.php file. The problem with symlinks is caused by moving from the use of cwd() in D7 to __DIR__ in D8. __DIR__ resolves symlinks.

Using

chdir(dirname(dirname($_SERVER['SCRIPT_FILENAME'])));

is fine for /core/install.php but it still can't find the settings.php file because that's found using get_conf_path which uses DRUPAL_ROOT which is set in bootstrap.inc using __DIR__ now.

If I change bootstrap.inc to

define('DRUPAL_ROOT', dirname(dirname($_SERVER['SCRIPT_FILENAME'])));

then it will work fine for /core/install.php but then when it moves to regular operation with /index.php it's one directory too high.

And that's just trying to get it to find settings.php. I haven't looked at how it finds the /files directory.

34: 1792310.patch queued for re-testing.

Here's a re-roll.

Does not include the change to the require_once in statistics.php, as we do require_once using __DIR__ all over the place. So if that causes problems, we should be changing that everywhere else as well.

Very strange, this might be a random fail:

ImageFieldDisplayTest.php, line 229
ImageFieldDisplayTest.php, line 251

array_flip(): Can only flip STRING and INTEGER values!
in FieldableDatabaseStorageController.php, line 212

43: 1792310-43.patch queued for re-testing.

Anyone want to RTBC?

#2262085: Random test failure in Drupal\image\Tests\ImageFieldDisplayTest

I stumpled upon this issue too, when symlinking the core directory.

My structure looks like:

core -> ../drupal/core
index.php
sites

I tried to apply the patch from #43, but as update.php is not present anymore, that patch failed.

As mentioned by @brianmercer in #39 , the current DRUPAL_ROOT implementation (using __DIR__) is not working for the same reasons this issue exists. I see a solution in reclaiming getcwd() for setting DRUPAL_ROOT. This way the calling script (install.php, rebuild.php, ...) are responsible for setting the working directory to the drupal root (as they currently already try to do). I tried to document that in extending the comment on define('DRUPAL_ROOT', ... );.

So I attach a rerolled patch

• without updatephp (as the file does not exist anymore)
• with the changes to defining DRUPAL_ROOT

Added #1540136: Simplify index.php by moving DRUPAL_ROOT to bootstrap.inc as related issue.

Yes, this in fact looks correct to me.

Looking back at the referenced issue this is how it was before, so - even if that has been closed for quite a while - the fact that this is how it is in previous Drupal versions should be proof enough that it's safe and works under any circumstances. The __DIR__ trick introduced there was neat but since it doesn't work with symlinks there's no reason we shouldn't just go back to what is proven to work.

The $_SERVER['SCRIPT_FILENAME'] has also been confirmed by enough people above.

So - although it obviously wouldn't for more people to take a look at this, as always - I see no reason why this shouldn't go in as is, for Rewiewed & tested by the community it truely is. :-)

Edit: I did upload a patch in this issue but that was a mere re-roll, so I don't think I'm violating any codex here...

This looks good to me - I was wondering about the scripts in core/scripts - I think they might need work too. They are tricky since you have no idea where the user is running them from :(

If you have no idea where the scripts are running from but you do know it's inside of drupal and your trying to find the drupal root I use this bit of code in D7 to accomplish the task.

  $webroot = str_replace('\\', '/', realpath(dirname($_SERVER['SCRIPT_FILENAME'])));
  while (!empty($webroot)) {
    if (file_exists($webroot . '/index.php') && strpos(file_get_contents($webroot . '/index.php'), 'menu_execute_active_handler();') !== FALSE) {
      break;
    }
    $new_webroot = str_replace('\\', '/', dirname($webroot));
    if ($new_webroot == $webroot) {
      // At the top level, exit as we're outside of where we should be.
      exit;
    }
    $webroot = $new_webroot;
  }
  chdir($webroot);

I do think that the patch in #50 is ready to go and any additional issues can be handled in a followup. So my vote is to put this issue back to RTBC.

More complete patch attached (tested already).

OK, so my extended patch may need further work, even if it worked in several tests with BOA/Aegir: https://github.com/omega8cc/drupal/commits/8.0.x-symlinks-support

+++ b/core/includes/bootstrap.inc
@@ -208,9 +208,16 @@
+define('DRUPAL_ROOT', getcwd());

Drupal might not always be run from the current working directly. Because of this, I'm against using getcwd().

Alternatively, you could use readlink() to resolve any symlinks. Here's an example:

define('DRUPAL_ROOT', readlink(dirname(dirname(__DIR__))));

It is an interesting idea to use readlink(foo)! But we should then probably wrap it within is_link(foo) to avoid problems? Here is a quick test on what happens when you use readlink(foo) against regular file and not a symlink:

quad:~# touch a
quad:~# ln -s a b
quad:~# ls -l {a,b}
-rw-r--r-- 1 root root 0 Dec 27 01:38 a
lrwxrwxrwx 1 root root 1 Dec 27 01:38 b -> a
quad:~# php -r "var_dump(readlink('b'));"
string(1) "a"
quad:~# php -r "var_dump(readlink('a'));"
bool(false)
quad:~# php -r "var_dump(readlink('c'));"
bool(false)
quad:~# php -v
PHP 5.5.20 (cli) (built: Dec 26 2014 21:49:06)

Unfortunately this doesn't work with symlinked /core

define('DRUPAL_ROOT', readlink(dirname(dirname(__DIR__))));

The reason is simple: it is not the root directory what can be symlinked, but rather /core, or /index.php etc. so we should rather try to detect if /core (or something else?) is symlinked:

if (is_link(getcwd() . '/core')) {
  define('DRUPAL_ROOT', getcwd());
}
else {
  define('DRUPAL_ROOT', dirname(dirname(__DIR__)));
}

Then also this is required to make it work with symlinked /core

    // Include our bootstrap file.
    if (is_link(getcwd() . '/core')) {
      $core_root = getcwd() . '/core';
    }
    else {
      $core_root = dirname(dirname(dirname(__DIR__)));
    }

Of course it doesn't make any sense to check if getcwd() . '/core' is a symlink if we don't want to trust getcwd() for some reason. But in my tests using getcwd() is much more reliable than unpredictable dirname(dirname(__DIR__)), or worse yet, ('..'), so why not to use it instead? It is already used in other places, like scripts and tests.

We could probably re-use some logic used in Drush to detect Drupal root in the symlinks-aware mode:

/**
 * Exhaustive depth-first search to try and locate the Drupal root directory.
 * This makes it possible to run drush from a subdirectory of the drupal root.
 *
 * @param
 *   Search start path. Defaults to current working directory.
 * @return
 *   A path to drupal root, or FALSE if not found.
 */
function drush_locate_root($start_path = NULL) {
  $drupal_root = FALSE;

  $start_path = empty($start_path) ? drush_cwd() : $start_path;
  foreach (array(TRUE, FALSE) as $follow_symlinks) {
    $path = $start_path;
    if ($follow_symlinks && is_link($path)) {
      $path = realpath($path);
    }
    // Check the start path.
    if (drush_valid_drupal_root($path)) {
      $drupal_root = $path;
      break;
    }
    else {
      // Move up dir by dir and check each.
      while ($path = _drush_shift_path_up($path)) {
        if ($follow_symlinks && is_link($path)) {
          $path = realpath($path);
        }
        if (drush_valid_drupal_root($path)) {
          $drupal_root = $path;
          break 2;
        }
      }
    }
  }

  return $drupal_root;
}

/**
 * Returns the current working directory.
 *
 * This is the directory as it was when drush was started, not the
 * directory we are currently in. For that, use getcwd() directly.
 */
function drush_cwd() {
  if ($path = drush_get_context('DRUSH_OLDCWD')) {
    return $path;
  }
  // We use PWD if available because getcwd() resolves symlinks, which
  // could take us outside of the Drupal root, making it impossible to find.
  // $_SERVER['PWD'] isn't set on windows and generates a Notice.
  $path = isset($_SERVER['PWD']) ? $_SERVER['PWD'] : '';
  if (empty($path)) {
    $path = getcwd();
  }

  // Convert windows paths.
  $path = _drush_convert_path($path);

  // Save original working dir case some command wants it.
  drush_set_context('DRUSH_OLDCWD', $path);

  return $path;
}

/**
 * Checks whether given path qualifies as a Drupal root.
 *
 * @param string
 *   Path to check.
 *
 * @return string
 *   The relative path to common.inc (varies by Drupal version), or FALSE if not
 *   a Drupal root.
 */
function drush_valid_drupal_root($path) {
  if (!empty($path) && is_dir($path) && file_exists($path . '/index.php')) {
    // Drupal 8 root. Additional check for the presence of core/composer.json to
    // grant it is not a Drupal 7 site with a base folder named "core".
    $candidate = 'core/includes/common.inc';
    if (file_exists($path . '/' . $candidate) && file_exists($path . '/core/misc/drupal.js') && file_exists($path . '/composer.json')) {
      return $candidate;
    }
    // Drupal 7 root. Additional check for the absence of core/composer.json to
    // grant $path is not core/ of a Drupal 8 root.
    $candidate = 'includes/common.inc';
    if (file_exists($path . '/' . $candidate) && file_exists($path . '/misc/drupal.js') && ((basename($path) != 'core') || !file_exists($path . '/../composer.json'))) {
      return $candidate;
    }
  }
  return FALSE;
}

wow, this has been languishing here for a while. I think with more d8 traction this is going to become much more of an issue for many.

As an example, I was changing all of my project's dependencies over to be installed via composer today, drupal/core being one of them, and attempting to move all the dependencies out of my site's git repo. In doing so, I need to symlink the core directory into place after I do a composer install during deployments. Works perfect, except for the fact that core doesn't discover the settings properly when it is symlinked from elsewhere on the filesystem.

I have tried out all the patches in this thread, none of which seem to resolve the issue for me. Here is my project's structure (truncating some of the output for brevity):

/var/www/html/current/
|-- composer.json
|-- composer.lock
|-- config
|   `-- prod
|-- docroot
|   |-- ...
|   |-- core -> /var/www/html/shared/docroot/core
|   |-- ...
|   |-- libraries -> /var/www/html/shared/docroot/libraries
|   |-- modules
|   |   |-- contrib -> /var/www/html/shared/docroot/modules/contrib
|   |-- profiles -> /var/www/html/shared/docroot/profiles
|   |-- robots.txt
|   |-- sites
|   |   |-- default
|   |   |   |-- files -> /var/www/html/shared/docroot/sites/default/files
|   |-- themes
|   |-- update.php
|   `-- web.config
|-- drush
|   `-- policy.drush.inc
|-- scripts
|   `-- composer
`-- vendor -> /var/www/html/shared/vendor

I am using https://github.com/drupal-composer/drupal-project for my composer template here, which allows me to get a lot of the code out of the git repo and share it across releases. Would be nice if we could do the same with drupal core.

Adding related issue.

Problem still exists. Updated patch also fixes DrupalKernel::guessApplicationRoot().

Related issue #1672986: Option to have all php files outside of web root. is less general than this one. It does not address the shared code scenario of #66.

Tricky. This issue is probably far from ready to commit in the current form as there are too many open questions:

• What about /core/scripts?
• Problems with tests.
• Increased reliance on getcwd which might not be set.
• $_SERVER["SCRIPT_FILENAME"] might not be set for some web servers?

Here is a much simpler patch. As far as I know it has no downsides for existing sites. It adds support for symbolic links when running the site via index.php. With this patch, the DrupalKernel class now correctly obeys the $app_root parameter.

It doesn't work for update.php, authorize.php, install.php or scripts - but if you are trying to lock down a site as is a likely case for moving the core directory out of the web root then you should most likely delete all those files. Instead run database updates via drush, code installs via composer, etc.

It is likely impossible to add tests for this patch because the test bot cannot create the required scenario where core the directory is a symbolic link.

Improved version of patch

Improved patch and issue summary update

Why not to try to use drupal/core-composer-scaffold approach where the web root property defined in the extras section.
That approach would be configurable per Composer template.

Why not to try to use drupal/core-composer-scaffold approach where the web root property defined in the extras section.

Thanks for the suggestion. However I think that solves a different problem. It seems to me that setting the web root property will move the web root to a different directory DD and then install core into DD. The idea of this issue is to take core out of the webroot.

I don't see how a composer plugin can solve this requirement. It's the job of composer to define where files will be installed. This issue is the next step: if the core files are installed outside of the webroot then core doesn't work.

Or have I missed something??

A lot of functionality tied up on the DRUPAL_ROOT: describes where the webroot and the core folder are located. For backward compatibility, we can define DRUPAL_ROOT constant based on the composer.json settings definitions. BUT that change also requires to provide additional info:

- web_root - related path to the folder where index.php, the other scripts, and other publicly accessible files are located. Currently, this value equal to the DRUPAL_ROOT;
- core_root - related path to the core package location. Currently, this value equal to the DRUPAL_ROOT. But if we move the core package to the vendor directory, then that will break paths for projects which directly relies on the core files, i.e., legacy include files;
- config_root - directory for site/multisite settings as they are currently located at sites/*/settings.php and related files (also yml config files can be stored here). It would be more securely if the configuration would be located out of web_root;
- assets_root - for files accessible for web request, contains the files located at sites/default/files (css, js, uploaded files)
- extentions_root - where the proper custom extensions (modules, profiles, themes) would be located. Extensions naming issues currently prevent moving extentions into the vendor directory, so this would be required first step in direction to move extensions out of web_root.

Benefits:
- definitions from composer file would be available immediately after including autoload file before initiating of kernel instance;
- more granular configuration which will allow moving the things around as that requires a particular project;

Conclusion: I know that the scope of the issue to change the way how DRUPAL_ROOT is defined, but I think Drupal has to introduce more specific location configurations to achieve that project flexibility as a lot of developers wants to achieve.

Conclusion: I know that the scope of the issue to change the way how DRUPAL_ROOT is defined, but I think Drupal has to introduce more specific location configurations to achieve that project flexibility as a lot of developers wants to achieve.

It seems like a fine idea but I would say it is a separate issue and a feature request - creating something new and more flexible likely needing many changes throughout the code and even perhaps in modules (maybe not BC).

This issue is a bug fix - taking the existing method and making it work better with a simple and safe change that can easily be committed now. It doesn't seem to in any way prevent what you want to do so perhaps you can support it?

In fact the suggestion in #84 seems much more like the related issue #1672986: Option to have all php files outside of web root..

Thanks for pointing that.
So, regarding the latest patch: I see that chdir calls no more uses related paths, and relay on the DRUPAL_ROOT value. And approach much simpler that in previous patches.
+1 for RTBC

Great thanks @voleger

+++ b/core/assets/scaffold/files/index.php
@@ -11,6 +11,7 @@
+define('DRUPAL_ROOT', __DIR__);
 $autoloader = require_once 'autoload.php';
 
 $kernel = new DrupalKernel('prod', $autoloader);

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -293,6 +293,9 @@ public function __construct($environment, $class_loader, $allow_dumping = TRUE,
     $this->root = $app_root;
+    if (!defined('DRUPAL_ROOT')) {
+      define('DRUPAL_ROOT', $app_root);
+    }

One thing that seems confusing is why propagate DRUPAL_ROOT everywhere? Given the changes to DrupalKernel - should we not be passing in the root to the DrupalKernel constructor. We'd then have way less defines around.

The other thing that reading this patch seems to indicate - is that we have two sources of truth as to the app root at the moment. DRUPAL_ROOT and the app root that's in the DrupalKernel - and can be set on construction. Having two sources of truth that have to be aligned makes this more painful than it should be. For example, I think that we really need to pass along the app root in \Drupal\Core\DrupalKernel::initializeSettings() to \Drupal\Core\DrupalKernel::findSitePath() - not doing that makes this more complex than it should be.

Having two sources of truth that have to be aligned makes this more painful than it should be

I absolutely agree with that! I just noticed we have a 3rd source of truth: \Drupal::root(). However it's not a situation created by this patch so we just have to deal with it as best we can.

One thing that seems confusing is why propagate DRUPAL_ROOT everywhere?

OK I get it now, thanks for explaining. The new patch minimises use of DRUPAL_ROOT.

The more I look the more instances I find!

This patch attempts to fix all places where $app_root has been defaulted ignoring tests and scripts.

Sorry for all the failures. I've done some more testing this time so hopefully it will work better.

This patch removes InstallCommand/ServerCommand because that's the same as scripts. I raised a separate issue for those #3176691: Allow scripts/tests to work with non-standard code structure.

Rerolled #94 patch against 9.2.x.
Anyway the changes from #94 +1 for rtbc

Great thanks @voleger I reviewed your reroll and agree this is RTBC.

Closed #3188703: index.php lets DrupalKernel guess the app root, which doesn't work if Drupal is being symlinked from a repo by Composer as a duplicate.

Tagging as DX, as fixing this will make it much simpler to work on Drupal core with the package symlinked in from a local repo by Composer -- see https://github.com/joachim-n/drupal-core-composer for details.

Couple of problems I can see:

1.

I don't think the change to core/modules/statistics/statistics.php will work:

> DrupalKernel::createFromRequest(Request::createFromGlobals(), $autoloader, 'prod', TRUE, realpath(''));

realpath() resolves symlinks. So when Drupal code is being symlinked into the webroot, realpath() gives us the real path of the core/modules/statistics/statistics.php file, and so the kernel is created with the wrong app root.

2.

FunctionalTestSetupTrait::prepareEnvironment() creates the test site folder inside the real path, and so the test uses the wrong app root:

  protected function prepareEnvironment() {
    // Bootstrap Drupal so we can use Drupal's built in functions.
    $this->classLoader = require __DIR__ . '/../../../../../autoload.php';
    $request = Request::createFromGlobals();
    $kernel = TestRunnerKernel::createFromRequest($request, $this->classLoader);
    // TestRunnerKernel expects the working directory to be DRUPAL_ROOT.
    chdir(DRUPAL_ROOT);
    $kernel->boot();

TestRunnerKernel::createFromRequest() is being called without an app root, so the app root will be deduced from the *real* location of the file, and won't be in the web root. The kernel then defines the app root based on that, and so the test site folder is created in the wrong place. The test then crashes because later on it *is* looking in the right place for a site folder, but can't find one.

This is important because symlinking Drupal core into a Composer project is a good way to work on core without Composer making changes to core files -- see https://github.com/joachim-n/drupal-core-composer

Other than that, this is working well with the setup from that Composer project.

Another problem I can see is that we have two different places that store the app root:

- the DRUPAL_ROOT constant, defined in core/includes/bootstrap.inc
- the $app_root used in DrupalKernel, which is either passed in or guessed, and which can be obtained with getAppRoot()

I'm looking to see if there's an issue anywhere to deprecated DRUPAL_ROOT in favour of getAppRoot(), and I can't see anything.

Adding #2590077: [meta] Decoupling drupal/core from predefined directory structure as a parent issue.

Another problem I can see is that we have two different places that store the app root:

I agree this is true, but it's not a new problem. This issue doesn't make things any worse than before so I don't see that it should stop this issue from moving forward.

I'm looking to see if there's an issue anywhere to deprecated DRUPAL_ROOT in favour of getAppRoot(), and I can't see anything.

The comment #89 from alexpott helped confirm for me that DRUPAL_ROOT should be considered intended for deprecation.

@voleger Thanks for the reroll. However as far as I can see it doesn't cover the comments in #100 so this issue is still "Needs work"

1. I don't think the change to core/modules/statistics/statistics.php will work:

Good point so this still needs fixing I think.

2. FunctionalTestSetupTrait::prepareEnvironment() creates the test site folder inside the real path, and so the test uses the wrong app root:

True. However this issue is intended not to cover changes to tests and I have updated the IS to make that clearer. There is a separate issue for that: #3176691: Allow scripts/tests to work with non-standard code structure. It's not obvious in that case even how to work out the app root.

Correction I think point 1 from #100 is in fact fine.

1. I don't think the change to core/modules/statistics/statistics.php will work:

I can see no way to make it work if statistics.php is accessed via a link.

See IS "proposed resolution":

A few key files will need to remain in the web root without the use of symbolic links - this allows working out the application root by calling __DIR__ within these pages.

> I can see no way to make it work if statistics.php is accessed via a link.

I had a thought about this -- we probably need a new PHP file that's scaffolded which acts as a broker for any PHP files inside modules that need to be linked to.

So the actual link would be web/direct_link.php, with a parameter that gets it to load statistics.php.

But that's going to be complex and I'm very happy to leave that to a follow-up!

Added some review comments to gitlab

> Do we need to require that root is always passed in? Should we deprecation not passing it in. This would allow contrib to update for this change too.

Yes, we should. Because in the situation where Drupal is being symlinked into the webroot, then the code in DrupalKernel (or indeed any file that isn't scaffolded into the webroot) has no way of discovering the webroot location. That's because PHP resolves symlinks for things like __DIR__ and __FILE__ and gives you their real location. (Well, I've just googled and found https://github.com/logical-and/symlink-detective/blob/master/src/Symlink... ... but let's not go there.)

And the consequence of that would be that guessApplicationRoot() is removed.

Many thanks @joachim and @alexpott. I'm going to drop out of following this one - I no longer have a project that needs it and you both know much more about it than I do.

I had a thought about this -- we probably need a new PHP file that's scaffolded which acts as a broker for any PHP files inside modules that need to be linked to. ... But that's going to be complex and I'm very happy to leave that to a follow-up!

That sounds clever. It could even be for more than just module code - potentially it could be used for any file that isn't already a scaffold file: install.php, authorize.php, etc. This would remove all restrictions, and even the entire core directory could be a symlink.

Do we need to require that root is always passed in? Should we deprecation not passing it in. This would allow contrib to update for this change too.

Yes, also I agree, however I expect we can't do it until after #3176691: Allow scripts/tests to work with non-standard code structure. The IS has a bullet for it under "related tasks", but I never got so far as to raise an issue.

> That sounds clever. It could even be for more than just module code - potentially it could be used for any file that isn't already a scaffold file: install.php, authorize.php, etc. This would remove all restrictions, and even the entire core directory could be a symlink.

Thanks! ... but I'm now wondering whether it's just inventing a whole secondary routing system! Which is bad. Given that the scaffolding system is already something that allows files to be declared to it, I think a better approach might be to allow modules to declare files for scaffolding. But this is all for a separate issue.

Aha! I've just discovered something interesting and useful!

  dump($class_loader->findFile(DrupalKernel::class));

This produces the location within the app root, that is, without resolving the symlink to its true location!

In my case, I get:

> "/Users/joachim/Sites/drupal-core-composer/vendor/composer/../../web/core/lib/Drupal/Core/DrupalKernel.php"

So anywhere we have the Composer autoloader, we can get the correct app root.

I've pushed a commit to the MR which fixes drupal_rebuild().

However, other parts of the code still need fixing.

I like using the class loader like this. It feels nicely elegant. It would mean we could do something like this in DrupalKernel:

      $app_root = static::guessApplicationRoot($class_loader);

and go back to the way the code looked like before this MR -- where most of the time, code lets DrupalKernel figure out the app root.

#111 looks pretty promising to me.

I wonder how much we can rely on the Composer autoloader findFile() method. We only have one place in core that currently does and this is the \Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter::mutateTestBase() so pretty obscure and not part of the regular runtime.

> I wonder how much we can rely on the Composer autoloader findFile() method.

Yeah... I was thinking that too, sadly.

All its docs say is:

> Finds the path to the file where the class is defined.

There's no detail as to whether a symlink is resolved or not. So the maintainers of Composer could legitimately say that symlinks being resolved or not is an implementation detail, and one that makes no practical difference to their API, since its purpose is to load files, and the OS won't care whether which version of the filepath you give it when loading a file.

I am also finding it quite complicated figuring out the DrupalKernel class -- it seems to have lots of different ways in, and static methods and instance methods that get called at various times.

For instance, here in drupal_rebuild(), a static method gets called when we've instantiated it:

  $kernel = new DrupalKernel('prod', $class_loader, TRUE, $app_root);
  $kernel->setSitePath(DrupalKernel::findSitePath($request));

We could pass the $app_root back in to findSitePath() as a parameter, which works.

But here in install_check_requirements(), we don't have a kernel instance:

      $site_path = empty($install_state['site_path']) ? DrupalKernel::findSitePath($request, FALSE, dirname(__DIR__, 2)) : $install_state['site_path'];

And then there are weird places like PublicStream:

        // If there is no kernel available yet, we call the static
        // findSitePath().
        $site_path = DrupalKernel::findSitePath(Request::createFromGlobals());

It would be helpful if the comment explained under what circumstances the kernel might not be available here!

All this means that out of the 3 places in DrupalKernel that static::guessApplicationRoot() is called, not all of them have access to the class loader.

However! I have a totally new plan!

We define the DRUPAL_ROOT constant in bootstrap.inc, using __DIR__ and the assumption that the file is in its standard place.

And bootstrap.inc is loaded by Composer's autoloader, by being specified as an autoload file in core\composer.json:

        "files": [ "includes/bootstrap.inc" ]

Suppose we add a new scaffolded file called 'drupal_root.inc'. It's scaffolded, so it gets put in to the /web folder when Drupal is installed by Composer. If it's defined as an autoload file to Composer, then it's loaded anytime the Composer autoloader is loaded (except while Composer plugins are running... you can't use dump() in a plugin, for instance, which is a total pain). So we could define DRUPAL_ROOT in that file, instead of in bootstrap.inc.

The thing is, I think it would need to be defined to Composer's autoload at the project level, because Composer will look for files relative to the package. And we don't want to load the package's copy of it, but the scaffolded copy. So that means that drupal/recommended-project would have to declared it as an autoload file.

I think this would work. Then what we'd have is:

1. Scaffolded drupal_root.inc declares DRUPAL_ROOT.
2. includes/bootstrap.inc declares DRUPAL_ROOT as a fallback, for projects that are set up with a non-standard composer.json
3. DrupalKernel relies on DRUPAL_ROOT if it's defined, and if not, guesses using the __DIR__ technique as currently

Argh, nope, that doesn't work.

Composer's autoload files get loaded with files from required packages coming first, and files from the project last -- see https://github.com/composer/composer/issues/7535#issuecomment-411992433

So in the generated Composer autoloader code, we get this:

    '2f69d3914119f042cca9e44442d5ce95' => $baseDir . '/web/core/includes/bootstrap.inc',
    '952683d815ff0a7bf322b93c0be7e4e4' => $vendorDir . '/chi-teck/drupal-code-generator/src/bootstrap.php',
    '801c31d8ed748cfa537fa45402288c95' => $vendorDir . '/psy/psysh/src/functions.php',
    '3917c79c5052b270641b5a200963dbc2' => $vendorDir . '/kint-php/kint/init.php',
    'cd774370d548d72500b76f2ed6e26769' => $baseDir . '/web/drupal_root.php',

and so bootstrap.inc gets loaded before drupal_root.php.

So drupal_root.php is workable, but we'd have to remove defining DRUPAL_ROOT from bootstrap.inc completely.

Or alternatively, maybe there's a way that it can be declared to the autoloader in drupal/core's composer.json? AutoloadGenerator::getPathCode() looks like there might be a way to do this, but WHY don't some developers write inline comments to explain their code???

Oh wait, I think this can work.

In core/composer.json:

        "files": [
          "../drupal_root.php",
          "includes/bootstrap.inc"
        ]

The files get autoloaded in the order we need them to be.

I've pushed a new MR with this approach: https://git.drupalcode.org/project/drupal/-/merge_requests/473

Though I think we can drop the fallback defining in bootstrap.inc, since both files are always going to be loaded.

Drat, but my MR doesn't work with @derEremit's script in #1, because in that setup, EVERYTHING is symlinked into the webroot, and all of Composer's files are outside it too.

In that setup, the only thing that's reliable is $_SERVER['SCRIPT_FILENAME']. But using that will mess up Drush. Though Drush crashes anyway when I run it in the webroot in @derEremit's setup!

Given #116 works how about we put the definition of DRUPAL_ROOT in autoload.php - and deprecate it being defined in bootstrap,.inc

Then we don't need a new file or changes to composer.json and all front controllers are suppose to use it. Does Drush use it... yep it does - it has \Drush\Config\Environment::loadSiteAutoloader()

> Given #116 works how about we put the definition of DRUPAL_ROOT in autoload.php - and deprecate it being defined in bootstrap,.inc

It won't work in a file that isn't bootstrapped.

I *think* for this issue there are two use cases:

A. The Composer project has the webroot within it (typically at /web), and Drupal core is symlinked into the Composer project. Example at https://github.com/joachim-n/drupal-core-composer
B. The Composer project has Drupal core installed normally, and is wholly outside of the webroot, with parts of it symlinked into the webroot. Example in the script in #1.
[C. Both use cases could co-exist, but I think for this issue it reduces to case B.]

The issue summary needs updating to explain these use cases, but I'll wait and see if there are more I have missed.

Where I think we are with possible techniques to deal with this is:

- using __DIR__ in any non-scaffolded file fails, because PHP always resolves symlinks in filename-related magic constants
- using __DIR__ in a composer-autoloaded scaffolded file (#116) works in use case A, but not in the use case B.
- using $_SERVER['SCRIPT_FILENAME'] works, even if the file being run is over symlinks (e.g. core/install.php), but it's not going to work with Drush. It's also quite fiddly as it needs to be setup in every single file that's an entry-point to Drupal
- Other things I may have missed

@joachim I don't understand why it wouldn't work in our weird autoload.php you have to load that file in order to bootstrap / autoload in the first place. It's always present in the Drupal's root. It's created by \Drupal\Composer\Plugin\Scaffold\GenerateAutoloadReferenceFile

Plus I wonder if we're over-thinking all of this. How about we have an environment variable that we check for and if that is set to something use that instead. That way was support all cases without having to adjust too much of the eco-system. Then we could make \Drupal\Core\DrupalKernel::guessApplicationRoot public and tell people to use that instead of any __DIR__ tricks.

> I don't understand why it wouldn't work in our weird autoload.php you have to load that file in order to bootstrap / autoload in the first place.

That would work in case A. I hadn't noticed that file -- it's hard to keep track of all the composer.jsons and autoload.phps! I'll make a MR for that later.

I don't think it would work in case B. Though I am increasingly thinking that we should say case B is unsolvable in the state as presented in the script in #1. I'm thinking instead, the sites folder needs to be kept in the Composer project folder, and Drupal's files folder needs to be explicitly placed in the webroot rather than inside sites/default. I'll experiment.

@alexpott are you sure it's [DRUPAL]/composer/Plugin/Scaffold/GenerateAutoloadReferenceFile.php?

I've done:

1. delete [WEBROOT]/autoload.php
2. put 'crash()' at the top of that file
3. done `composer update --lock`
4. the autoload file is restored. Nothing has crashed.

There is also vendor/drupal/core-composer-scaffold/GenerateAutoloadReferenceFile.php. Is it maybe that one? (I've not tried it yet; need to go make dinner.)

@joachim the two are the same... in order to make changes locally you need to change both. The composer scaffold plugin is built from the code in /core but ends up in vendor. And yep, it is confusing.

On a core development checkout you'll also need to change the root autoload.php for good measure. Luckily this file never changes :D

Ok! New MR https://git.drupalcode.org/project/drupal/-/merge_requests/483 with the approach of defining DRUPAL_ROOT in our own autoload.php, and having DrupalKernel use that.

This is working for me for case A.

For case B, I can get it working if I don't copy the sites folder into the webroot. But then the files folder won't be accessible over the web! I really don't what to do about this case. I think maybe the first thing is to get detailed instructions for setting up the whole project outside the webroot that are newer than 2013.

Also, this is going to be tricky to test, because of the changes in drupal/core-composer-scaffold.

> The composer scaffold plugin is built from the code in /core but ends up in vendor.

AFAICT, the drupal/core-composer-scaffold is being read from the d.org git repo. The base composer.json in Drupal defines local Composer repos in composer/Plugin/ProjectMessage and composer/Plugin/VendorHardening, but not composer/Plugin/Scaffold, which is what would make symlink it from the installed drupal core files. In my case, I added this to the project's root composer.json repositories section:

        {
          "type": "path",
          "url": "repos/drupal/composer/Plugin/Scaffold"
        }

I *think* this is ready. (I mostly find time to work on this issue with kids TV in the background, my focus is not perfect.)

It fixes the case where Drupal is symlinked into the Composer project, which is important because it makes developing Drupal core itself much easier: see https://github.com/joachim-n/drupal-core-composer for reasons.

It doesn't fix the case with the script in #1, but the issue summary says:

> #1672986: Option to have all php files outside of web root. - this issue does not solve that case but is a pre-requisite.

Also, the #1 script doesn't satisfy the case of moving all PHP files out of the webroot, as you end up with a webroot with the core, modules, profiles, and themes folders symlinked in. So all the code is effectively accessible over the web.

Currently, moving all files outside of the webroot isn't possible. The problem is that the concept of 'app root' means too many things:

1. The folder beneath with Drupal can find the site folder and the settings.php file
2. The folder beneath which Drupal can find the public files.
3. The folder beneath which Drupal can find its core files, and contrib modules and themes

Moving PHP files out of the web root means that we need a concept of 'app root' which is used for 1 and 3, and a concept of 'web root' which is used for 2.

I think the next step is probably to unpick those two.

I thought I'd seen an issue about reading the locations of the web root and the app root from the Composer scaffold settings, which seemed like a good idea to me, but on rereading it, I misunderstood the issue (#3084369: Look up scaffold [web-root] from installation location of drupal/core).

Ah yes, testing this!

To test this with the symlinking set up, I had to tweak the project composer.json I use for symlinking core. This is because we need the drupal/core-vendor-hardening Composer plugin to come from the Drupal clone which is on this issue's branch, and by default, it will be downloaded from the main package repository. (Related issue: #3207499: source the drupal/core-vendor-hardening plugin from the drupal project, or document why not.)

Here's the composer.json file I used to set up the project. Instructions for setting it up are the same as for https://github.com/joachim-n/drupal-core-composer.

I have no idea how we go about writing automated tests for symlinking Drupal. Well, I do, but they seem complicated and fiddly to me: get PHPUnit to create a new project folder inside the test site folder (so it gets cleaned up after the test) and write a composer.json file there, which defines a Composer repository to symlink in the git clone of Drupal that we're running in. The do composer install in that folder, check that works, check Drupal's installation process can be run in browser.

Oops. Here's the file.

Argh, nope, !483 / branch 1792310-drupal-root-in-autoload doesn't work with Drush. I'd tested it, but before I added back the guarded definition of DRUPAL_ROOT in includes/bootstrap.inc.

This is what goes wrong:

- Drush's entry point needs to bootstrap Drupal
- it starts off by loading Composer's autoload file in vendor/autoload.php
- because the drupal/core composer.json defines includes/bootstrap.inc as an autoload file, that gets loaded. DRUPAL_ROOT isn't defined yet, so the guarded code for defining it runs
- loadSiteAutoloader() in vendor/drush/drush/src/Config/Environment.php then includes the Drupal scaffolded web/autoload.php file, which then complains that DRUPAL_ROOT is already defined

We can't expect Drush, or indeed any external CLI script to load the scaffolded web/autoload.php file, because they shouldn't assume they know where Drupal is, and in order to know where Drupal is, they need to ask Composer, and to ask Composer they need to start up the autoloader.

@alexpott what cases are covered by the guarded definition you wanted in includes/bootstrap.inc?

@joachim good question. Perhaps we can move the guard out of bootstrap.inc and into \Drupal\Core\DrupalKernel::__construct() after the app root is guessed. This way anyone who is booting Drupal will have this set for sure even if they've avoided our odd autoloader thingy (not using our odd autoloader is the case I'm concerned about).

Fixed Custom Commands Failed.

I think one of several follow-ups to this issue is going to be to define and document *one* correct way to run Drupal from a CLI.

> This way anyone who is booting Drupal will have this set for sure even if they've avoided our odd autoloader thingy (not using our odd autoloader is the case I'm concerned about).

Drush uses our weird autoloader, but to use our weird autoloader, Drush (or another script) has to first find it. So it's circular -- our autoloader will define DRUPAL_ROOT, but you need to know that to load it.

Drush uses webflo/drupal-finder which tries various things to figure our where Drupal is relative to Composer.

I was starting to think we need to go back to branch 1792310-drupal-root-scaffolded-file, the approach of having DRUPAL_ROOT defined in a file that Composer includes in the autoload process, which gets scaffolded to the web root. That way we know that we set it up as soon as a script interacts with Composer.

However, there's a chicken and egg situation there too: defining the drupal_root.php file in composer.json as "../drupal_root.php" is making the assumption that drupal/core sits one directory below the webroot.

Really, the only authority on where things are is Composer, since the project root's composer.json defines where files got put in the drupal-scaffold property. But how to get that out... arrgh!

Really, the only authority on where things are is Composer, since the project root's composer.json defines where files got put in the drupal-scaffold property. But how to get that out... arrgh!

Yeah - this is one of those. I think the current solution is okay. Anything that has to find Drupal outside of Drupal has to locate our autoload.php and include that. Them's the rules. And that allows us to define DRUPAL_ROOT. This solution coupled with the other changes will allow you to symlink core and if you're doing it differently will you might not support symlinks but you'll be no worse off than HEAD today.

> I think the current solution is okay.

Yup, agreed. One step at a time. This feels like a really complex area to unravel.

I've filed follow-ups with some of the ideas I've had:

- #3208975: split the concept of DRUPAL_ROOT/app root into app root and Drupal web root
- #3208979: Provide a single entry-point to Drupal for CLI scripts

Thanks for the MR review @alexpott. Will have a look now.

From the MR review:

> I think we shouldn't rename [guessApplicationRoot] in this issue. We can file a follow-up to deprecate and rename.

Making a note here of that here for another follow-up.

I've made both the requested changes to the MR.

Also tested the current code with:

- core/rebuild.php
- update.php
- Drush cr
- Drupal Console rr

to check that they are all correctly picking up the sites folder and the contrib modules with a Drupal Core symlinked setup.

Just an update about the Composer project I've been working on that installs Drupal core from a git clone over a symlink: I've made into a fully-fledged Composer project, so it can be installed simply with `composer create-project joachim-n/drupal-core-development-project`. There's a Composer script that takes care of cloning Drupal core, so it's a single command setup and then drush si to install Drupal.

I've also moved it to a new repo with a better name -- https://github.com/joachim-n/drupal-core-development-project

@joachim: I'm afraid the Build Successful return of TestBot isn't quite accurate (#3208931: TestBot returns with "Build Successful" when there are too many test failures to report).

If you look at the full log of console output of the test-run, there's not much reason to rejoice... :/

Urgh drat.

The tests are failing because:

1. PHPUnit loads the core/tests/bootstrap.php file specified in the phpunit.xml file. This is loaded from its real location, not symlinked, because PHPUnit's file loader resolves symlinks.
2. In that file, drupal_phpunit_populate_class_loader() does require __DIR__ . '/../../autoload.php'; Which in a symlinked setup, gets us the wrong file.

After that I'm not sure....

New branch with a new approach coming...

The approach of defining DRUPAL_ROOT inside our special autoload.php has a problem because in some circumstances it's circular: our special autoload.php is scaffolded into the web root, and defines the location of the webroot.

If you're coming into Drupal with an HTTP request, then you (usually) start off in the webroot with index.php, that includes our special autoload.php, and then it's fine.

But if you're a script, or phpunit, you come in from your own package. Scripts such as Drush and Drupal Console have to guess at where the webroot is in order to load our special autoload.php. At which point, defining DRUPAL_ROOT is too late to be useful. Guessing the location of the webroot from outside of it, or guessing how to get to Composer from within the webroot, is the problem this issue needs to fix.

So, re-thinking this from first principles:

- the only authority on where packages are located is Composer, because it puts them there
- the only authority on where the webroot is located is the project root composer.json, which defines it, and also our scaffolding Composer plugin, because it reads it from composer.json and then puts scaffolded files there
- any entry point, whether it's an HTTP request, a script, or phpunit, always starts by loading the Composer autoloader.

So:

1. Write a .inc file during scaffolding which defines DRUPAL_ROOT
2. Ensure this gets loaded during Composer's autoloading.
3. The file has to be in drupal/core, not in webroot, because Composer doesn't know where the webroot is (at least, not without reading the composer.json file which we don't want to be doing every single request)

New MR for this approach: https://git.drupalcode.org/project/drupal/-/merge_requests/591

This approach also sets groundwork for #3208975: split the concept of DRUPAL_ROOT/app root into app root and Drupal web root, because we can easily define more constants in that file, and #3208979: Provide a single entry-point to Drupal for CLI scripts, because once scripts load the Composer autoloader, they can find the Drupal webroot.

I've run a few tests both on a symlinked set up and with a flat git clone on which `composer install` is run.

Fixed the various coding style errors.

And cherry-pick-squashed all the DrupalKernel stuff from the other branch which I'd completely forgotten we need here too :/

> I have no idea how we go about writing automated tests for symlinking Drupal. Well, I do, but they seem complicated and fiddly to me: get PHPUnit to create a new project folder inside the test site folder (so it gets cleaned up after the test) and write a composer.json file there, which defines a Composer repository to symlink in the git clone of Drupal that we're running in. The do composer install in that folder, check that works, check Drupal's installation process can be run in browser.

Haha! Since I wrote that comment, I have noticed that there are tests in Drupal that do pretty much exactly this, to test the Composer project templates which are in core!!

It seems like killing DRUPAL_ROOT for the app.root service parameter is a better approach than yet another scaffolding file.

That's worth exploring in a new branch, definitely.

What I like about the scaffold file technique is that it preserves DRY - the app root is defined only once in composer.json.

But if Drupal is being used as a library by something else, the scaffolding plugin probably won't even be in use.

Thinking about it some more, I don't see how an app.root service parameter could work in the symlinked repo scenario.

In order to get the service parameter, you need the default.services.yml file. And in order to get that file, you need the sites folder. To know where the sites folder is, you need the app root.

New idea! I've found something quite nice in the Composer API (https://getcomposer.org/doc/07-runtime.md):

// returns an absolute path to the package installation location if vendor/package is installed,
// or null if it is provided/replaced, or the package is a metapackage
// or throws OutOfBoundsException if the package is not installed at all
\Composer\InstalledVersions::getInstallPath('vendor/package');

Which means that in DrupalKernel, we can do this:

// Returns "/Users/joachim/Sites/drupal-core-composer/vendor/composer/../../web/core"
$drupal_core_installed_path = \Composer\InstalledVersions::getInstallPath('drupal/core');
// Sets $app_root to "/Users/joachim/Sites/drupal-core-composer/vendor/composer/../../web/"
$app_root = dirname($drupal_core_installed_path);

This gets us **exactly** what we want! This is the location where Composer has installed drupal/core, and that location is PROJECT_ROOT/web, because that's the location that is defined in composer.json. (Admittedly, there are some '..' in the path, but those are fine, right?)

However... this API is only available in Composer 2.1 :(

But it does mean that there is a really simple fix:

- DrupalKernel checks whether Composer\InstalledVersions::getInstallPath() exists, and if so, uses that to determine app root / DRUPAL_ROOT
- If it doesn't, because Composer is old, use the current technique which assumes a standard code structure
- we say that if you want a non-standard code structure, you need to install Drupal with Composer ^2.1

Could we at some point require a minimum version of Composer? There doesn't seem to be any Composer version requirements at the moment.

@joachim yes please!
#156++

I found this blogpost hinting about Composer v1 EOL - https://blog.packagist.com/deprecating-composer-1-support/

We hinted in the release announcement that Composer 1.x was pretty much EOL

And some more info about Composer v1 EOL - https://github.com/composer/composer/issues/10340

I realised after posting that while this covers the 'Drupal symlinked into web/core' case, it doesn't help at all for the 'Drupal installed into vendor/' case.

There's no simple way to handle that one.

To recap/rubberduck, there are 3 4 kinds of entry point into Drupal:

- A. HTTP request to the index.php file which is scaffolded into the app root
- B. running tests
- C. code in a Composer package (e.g. Drush or Console) bookstraps Drupal
- D. HTTP request to core/modules/statistics/statistics.php (yay! special case!)

If Drupal is installed by Composer in web/core then in all cases, asking Composer 'where did you put Drupal?' is fine.

If Drupal is installed by Composer in vendor/, then the only piece of information about the location of the app root is the definition of the scaffold location in composer.json.

We can get that by:

- reading composer.json (ugly)
- writing a file to a fixed location during the scaffolding process (also ugly)
- installing some sort of package to the scaffold location, so we can ask Composer about that package.

I'm going to investigate the 3rd option. We currently put drupal/core into web/core/, but there's no package installed into web/. Could we have a dummy, empty package which does there? Or would Composer complain about web/core/ being inside web/?

I wrote a custom composer plugin as an experiment for drupal-finder a while ago. Its currently hosted on github: https://github.com/webflo/drupal-finder-plugin

It stores additional metadata as a PHP class, similar to InstalledVersions::getInstallPath() and https://github.com/Ocramius/PackageVersions. I think it possible to compute all necessary information during installation of this plugin. It reacts on composer install and update as well.

Give it a try. "composer require webflo/drupal-finder-plugin:1.x-dev".

The code: https://github.com/webflo/drupal-finder-plugin/blob/1.x/src/DrupalFinder...

The generated class is called \DrupalFinderPlugin\Info

Cool, thanks!

We could do the same thing with the drupal/core-composer-scaffold plugin.
It would introduce a foreign file into the git repository in the setup where core is symlinked in by Composer. I suppose that could be handled with a .gitignore that's in the repository as well.

For setups where the drupal/core-composer-scaffold plugin is absent, we could require that the project do its own thing to define that same class.

However... this API is only available in Composer 2.1 :(

Looks like it's worth adding composer-runtime-api ^2.0 to core/composer.json given the docs say

 * This class is copied in every Composer installed project and available to all
 *
 * See also https://getcomposer.org/doc/07-runtime.md#installed-versions
 *
 * To require its presence, you can require `composer-runtime-api ^2.0`

We should do this for Drupal 10 in another issue.

Also - goes without saying but we need to performance test this. This is going to result in loading vendor/composer/installed.php on every request. Which with core alone is nearly 2000 lines long... and on a decent size project it is double that. Loading this file for every request doesn't feel right.

I've had a go at installing drupal/core in vendor/, with some quick hack in DrupalKernel to make it set DRUPAL_ROOT correctly.

It falls over pretty quickly, because DrupalKernel::compileContainer() assumes that the app root, that is, the place where index.php is scaffolded to, is also where the core folder is located:

    foreach (['Core', 'Component'] as $parent_directory) {
      $path = 'core/lib/Drupal/' . $parent_directory;
      $parent_namespace = 'Drupal\\' . $parent_directory;
      foreach (new \DirectoryIterator($this->root . '/' . $path) as $component) {

It's trying to take directory listings of web/core/lib/Drupal/Core, because we've defined $app_root as 'web/', but finding nothing, because the folder it's trying to list is at vendor/drupal/core/lib/Drupal/Core.

So, I think the scope of this issue needs to be changed: we can't fix the drupal/core in vendor case until later on, in #3208975: split the concept of DRUPAL_ROOT/app root into app root and Drupal web root. This issue should nonetheless allow for that case to be able to correctly set the app root, even if it's not useful yet.

My latest plan was this:

- scaffold plugin generates a PHP file during the scaffold, written to the plugin's folder
- this file is registered as a "file" autoload with Composer, via a wrapper file (because otherwise Composer will complain when it hasn't been written yet)
- the file defines a namespaced constant, \DrupalLocation\APP_ROOT. The scaffold plugin sets the value of the constant to the scaffold location, which it knows during the scaffold process
- DrupalKernel uses this constant to set $app_root if it's defined, and uses the current 'jump up 2 dirs' logic if it's not defined (in the case there the scaffold plugin is not present)

And how this works out for different setups:

- standard setup with scaffolding: the scaffolded constant is used
- core symlinked in by Composer: the scaffolded constant is used
- plain clone of core: 'jump up 2 dirs' is used
- weird nonstandard setup, such as core in vendor/ : it's up to you to define \DrupalLocation\APP_ROOT yourself

This was working nicely!

Then I started looking at how we deprecate DRUPAL_ROOT, and I got stuck on the problem that Composer doesn't guarantee the order of autoload files. We have:

- core/includes/bootstrap.inc, registered as an autoload file by drupal/core, defines DRUPAL_ROOT
- drupal_location_loader.php, registered as an autoload file by drupal/core-composer-scaffold, defines \DrupalLocation\APP_ROOT

and we don't know which one comes first, but we really need \DrupalLocation\APP_ROOT to get defined first!

So unless anyone has any other ideas, I am going to tweak this so the generated file defines a class. That means that in core/includes/bootstrap.inc we can do class_exists(), and get the APP_ROOT constant.

I liked the idea of making it a namespaced constant, because it means that when we get to #3208975: split the concept of DRUPAL_ROOT/app root into app root and Drupal web root we can define constants in the same namespace but in different parts of the code.

I'm tempted to cheat and put the constant definition in the class file but outside of the class. But that would be hacky, right???

More rubber-ducking...

We need to write something during scaffolding which records the location of Drupal's app root.

This needs to be:
- something that is either automatically loaded or loadable by Composer, so that things like Drush and Console get it
- something that loads before Drupal's core/includes/bootstrap.inc, because otherwise that will set DRUPAL_ROOT
- it has to have the locations as strings, not use magic constants like __DIR__ because those won't work with symlinks

The simplest thing is to have the scaffolding plugin write a file. (I've tried this in various branches - 1792310-drupal-root-in-scaffolded-include-file, and 1792310-drupal-root-scaffolded-file but both get it wrong.)

The location file can't be in a scaffolded location, because then Composer autoload can't find it -- we can't make assumptions in composer.json's autoload config about where it's been put.

So it has to be written into a package's folder, like @webflo's plugin (see #159), and the package can have a .gitignore which says to ignore it.

The options are:

- autoload file defining a constant: this is tricky because Composer includes autoload files in dependency order (see https://github.com/composer/composer/issues/10509). So the only way to get this included before core's core/includes/bootstrap.inc (which is also an autoloaded file) is to have it in something that drupal core requires. That rules out the scaffolding plugin, or putting it in the project's composer.json.
- autoload class: I wasn't sure about this because then a nonstandard project would have to define that class itself to set the locations.

However, instead of this location class being a new API for projects to use, I am now thinking we could simply use the existing API, which is the "locations" config in the project composer.json.

So my new plan is: we add a new Composer plugin which is inside core, whose sole job is to write the locations file into itself. Make the plugin required by core so that the autoload file loads before core/includes/bootstrap.inc. That means it's always present, even in projects that don't use the scaffold plugin. And nobody else needs to worry about defining it, as to change the location values you set them in composer.json.

So my new plan is: we add a new Composer plugin which is inside core, whose sole job is to write the locations file into itself. Make the plugin required by core so that the autoload file loads before core/includes/bootstrap.inc. That means it's always present, even in projects that don't use the scaffold plugin. And nobody else needs to worry about defining it, as to change the location values you set them in composer.json.

Sounds reasonable.

One thing I'm wondering is are we using DRUPAL_ROOT etc too much. Maybe we don't need to use it - otherwise other applications would have this problem.

> One thing I'm wondering is are we using DRUPAL_ROOT etc too much. Maybe we don't need to use it - otherwise other applications would have this problem.

I had a quick look at other CMS's installation docs.

- Wordpress doesn't use Composer, so it controls its own file structure
- Joomla also doesn't use Composer
- Laravel installs with Composer, but I don't see any special locations declared, so it looks like its extensions just go in /vendor

The reason we have DRUPAL_ROOT is that we're supporting multiple ways of installing Drupal with different structures -- the recommended template, the legacy template, and plain git clone of core.

New MR ready for review: https://git.drupalcode.org/project/drupal/-/merge_requests/1922

I've set the old MRs which are mine as drafts.

And I've rewritten the IS to explain in depth the problem and how the MR works. I hope it's clear. This is a complex topic, which has taken me a long time to understand!

Not sure which component is best for this, but probably not the installer.

Both of the failing tests are passing for me locally with a git clone of the branch, run like this:

1. clone the branch
2. do composer install in the root of the git clone
3. set up phpunit.xml, pointing it at an empty DB
4 vendor/bin/phpunit core/modules/system/tests/src/Functional/UpdateSystem/RebuildScriptTest.php or vendor/bin/phpunit core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php --filter=testMinimumStabilityStrictness

These two tests also pass if I run then with run-tests.sh:

1. clone the branch
2. do composer install in the root of the git clone
3. install Drupal
4. install simpletest module
5. php core/scripts/run-tests.sh --url http://localhost:8888/drupal-test-install-git-clone-1792310 --dburl mysql://drupal:password@localhost/drupal-1792310-clone --file core/modules/system/tests/src/Functional/UpdateSystem/RebuildScriptTest.php` or php core/scripts/run-tests.sh --url http://localhost:8888/drupal-test-install-git-clone-1792310 --dburl mysql://drupal:password@localhost/drupal-1792310-clone --file core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php

Since I can't reproduce the test failure in RebuildScriptTest locally, I've been trying to get debugging info out of Drupal CI using a testing issue -
#3271051: Testing issue for Joachim, ignore!.

It's not going well -- I'm chucking print_r() statements into patches, but not always getting results when the patch is tested :(

What I think I know is this:

This line fails:

> $this->drupalGet('/container_rebuild_test/module_test/module_test_system_info_alter');

because the page reports that the module_test module is still in its original location, in core/modules/system/tests/modules/module_test rather than in SITE_DIR/modules/module_test.

The copy command is this:

> $file_system->mirror('core/modules/system/tests/modules/module_test', $this->siteDirectory . '/modules/module_test');

It works locally, and IIRC on Drupal CI debugging shows that the module gets copied to the site directory for the test site.

Debugging in ExtensionDiscovery seems to show however that SITE_DIR/modules is NOT getting picked up as a module folder.

That's about as far as I've got.

OK, the previous patch does not work. The issue here is Apache webser behavior with mod_dir:

A "trailing slash" redirect is issued when the server receives a request for a URL http://servername/foo/dirname where dirname is a directory. Directories require a trailing slash, so mod_dir issues a redirect to http://servername/foo/dirname/.

rebuild.php is not adding a trailing slash when redirecting to a front page when website is run in a subdirectory (like on drupalci)

Note to eventual committer - please credit @Taran2L and @bbrala for lots of time figuring out the test failure on slack.

After investigating a little, the problem with the tests is in rebuild.php.

Settings::initialize($app_root, DrupalKernel::findSitePath($request), $autoloader);

This breaks the test. In my local env i fixed this by making DrupalKernel::getApplicationRoot() public and using that as first argument in the initialize command. Suddenly it knows what to do. The fact it uses $app_root = dirname($_SERVER['SCRIPT_FILENAME'], 2); here is a problem in a symlinked env (as the ci is, and my local env is ;))

I'm not gonna push to the branch since i'm not sure making that public is feasable. I'll let that be decided by the people working on this :)

For completeness sake;

diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
index de3f74889a..5dcf62b4f1 100644
index de3f74889a..5dcf62b4f1 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -340,7 +340,7 @@ protected static function guessApplicationRoot() {
    *
    * @see \Drupal\Composer\Plugin\Scaffold\GenerateAutoloadReferenceFile
    */
-  protected static function getApplicationRoot() {
+  public static function getApplicationRoot() {
     if (class_exists(DrupalLocation::class)) {
       return DrupalLocation::APP_ROOT;
     }
diff --git a/core/rebuild.php b/core/rebuild.php
index e957440905..0d83ce39c8 100644
--- a/core/rebuild.php
+++ b/core/rebuild.php
@@ -11,12 +11,14 @@
  */

 use Drupal\Component\Utility\Crypt;
+use Drupal\Composer\Plugin\Locations\DrupalLocation;
 use Drupal\Core\DrupalKernel;
 use Drupal\Core\Site\Settings;
 use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;

+
 // Use SCRIPT_FILENAME rather than the current filename so that symlinks are not
 // resolved.
 $app_root = dirname($_SERVER['SCRIPT_FILENAME'], 2);
@@ -32,7 +34,7 @@
 DrupalKernel::bootEnvironment();

 try {
-  Settings::initialize($app_root, DrupalKernel::findSitePath($request), $autoloader);
+  Settings::initialize(DrupalKernel::getApplicationRoot(), DrupalKernel::findSitePath($request), $autoloader);
 }
 catch (HttpExceptionInterface $e) {

@Taran2L figured it out on slack:

The problem is that when the WHOLE of the project is symlinked, and the site run from that symlink (which is how Drupal CI sets up Drupal, for historical reasons, see #2838194: Run out of real subdirectory, not symlink), TWO different cache IDs are getting used for the container!

- a load of index.php with a cold cache sets a cid of service_container:prod:9.4.0-dev::Darwin:a:1:{i:0;s:85:"/PATH/TO/SITE/sites/default/services.yml";}
- and rebuild.php creates a cid of service_container:prod:9.4.0-dev::Darwin:a:1:{i:0;s:98:"/PATH/TO/SITE/subdirectory/sites/default/services.yml";}

And there's a simple way to fix that.

So I think the MR might be green now!

Thank you SO MUCH to @Taran2L and @bbrala for their help on slack over the last few days!

It would be great to see this in drupal:10.0.0. Needs reroll.

Made a new branch and rebased onto 10.0.x, opened a new MR.

seems like changes require running the following command
composer update --lock

Done! Thanks for the review!

Drupal\Tests\ComposerIntegrationTest passed!

I think this change is still going to be difficult. As far as I can see this change means that you have to run composer in production because it is writing absolute paths to the file. Not all projects are set up like this. It means your codebase is no longer portable without running composer commands. That's a very big change to make. Also writing to a code path like composer/Plugin/Locations/DrupalLocation.php is a big change.

> As far as I can see this change means that you have to run composer in production because it is writing absolute paths to the file

I'll change it to a relative path.

> Also writing to a code path like composer/Plugin/Locations/DrupalLocation.php is a big change

We have to write to a location that's loadable by class autoloading. Where would you rather it be written to?

I've had a quick play around with it, and I think that writing a relative path means that we have to do #3208975: split the concept of DRUPAL_ROOT/app root into app root and Drupal web root as part of this issue.

That's because sometimes code in Drupal wants the root to load things in itself, and sometimes code in Composer packages wants the Drupal root to load Drupal. Those two things work if the root is absolute. If it's relative, it breaks because those two things don't have the same working directory.

> Also writing to a code path like composer/Plugin/Locations/DrupalLocation.php is a big change

Thinking about this, the only options are:

- inside a package, because then Composer has it in its autoload
- in the root of the project, with an autoload entry in root composer.json
- in a dedicated subfolder of the root of the project, e.g. '/locations', with an autoload entry in root composer.json
- in the scaffolded Drupal, but then we have to violate DRY by having an autoload entry in root composer.json which repeats the extra>drupal-scaffold>locations value in the same file

@alexpott which do you prefer / dislike the least?

Rebased to 9.4 MR on 9.5.

> @alexpott which do you prefer / dislike the least?

@alexpott wrote on Slack:

> My quick gut feel is: I'm not sure it is an easy choice, but Drupal root is at least consistent with other files. This makes me wonder if we should add this to the autoload.php that we write already.

I've updated both MRs to write to the Composer root.

Using the autoload.php that we write is fine for Drupal code to find our DrupalLocations class, but doesn't work for Composer packages such as Drush, which also need to have it loaded.

The test failure is this:

22:15:53 In GenerateLocationsClass.php line 146:
22:15:53
22:15:53 file_put_contents(/var/www/html/DrupalLocation.php): Failed to open stream:
22:15:53 Permission denied
22:15:53

Looks like our CI isn't letting the Locations plugin write to the project root. Any ideas why?

There's a ton of documentation on how code interacts but what problem does this solve? How would I recreate this problem? The original issue summary sounds like a support request but the people working on this have me thinking its a bigger problem but I can't figure out what the expected outcome is.

> There's a ton of documentation on how code interacts but what problem does this solve?

Non-standard installations of Drupal which place core somewhere else, or install it in a different way.

Any thoughts on how to rewrite this line in the IS to be clearer?

This is important because the standard way of developing a Composer package, by defining a path repository in a project to symlink a git clone of the package, isn't currently possible with Drupal core.

> How would I recreate this problem?

Try to install drupal/core for development in a Composer project using the path repository technique: see https://medium.com/pvtl/local-composer-package-development-47ac5c0e5bb4

I made a Composer template to make this possible but it's basically a heap of hacks to work around the problem here. See https://github.com/joachim-n/drupal-core-development-project

The D10 MR is passing tests.

The D9 MR is failing because it's not managing to write the DrupalLocations class into the Composer project root. I don't understand how the D10 branch is managing to do that where the D9 branch is failing. The core/drupalci.yml file is identical in both branches.

Unless anyone has any ideas for how to fix this, I'm tempted to just drop the D9 branch, as I doubt this issue is going to land on D9 anyway!!

Agreed with #196, the D9 test run shows the following:

02:57:51  Drupal app root assumed to be the Composer project root, /var/www/html.
02:57:51  
02:57:51  In GenerateLocationsClass.php line 152:
02:57:51                                                  
02:57:51    The directory /var/www/html is not writable.  

and it goes downhill from there, whereas the D10 test run says the following:

05:47:57  Drupal app root assumed to be the Composer project root, /var/www/html.
05:47:57  Writing Drupal locations class to /var/www/html/DrupalLocation.php.

Thoughts, anyone?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

New branch & MR for 11.x

Have not reviewed.

But appears to have composer error and can't trigger retest.

Fixed the errors.

I only found one real question in regards of a missed dirname(__DIR__, 2); in one of the files. There is other places we might want to use our new found power instead of some of the references through __DIR__. Most seem in tests though which i'd not touch probably.

I've run through this whole issue and went through the MR intensely. This seems like an elegant solution that will enable us to take a step forward in how core is structured.

Resolved all points.