Problem/Motivation

The Composer Scaffold Plugin generates the wrong relative path to the vendor directory in the generated autoload.php file for any composer.json file that defined [web-root] at any location other than the project root. The reason for this is that the code is incorrectly calculating the relative path to the vendor directory starting from the directory that the autoload file is stored in. This produces a sort of off-by-one-error in the filesystem, and the wrong number of '..'s appear in the resulting path.

Proposed resolution

Calculating the relative path to the autoload file itself (rather than its parent) fixes this problem.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

The Composer Scaffold Plugin was generating the wrong relative path to the vendor directory in the generated autoload.php file for any composer.json file that defined [web-root] at any location other than the project root. The correct vendor path is now written to autoload.php regardless of the value of [web-root].

Original Report

(this is my first post of a Drupal.org Issue ever)

I was tasked in our Team to move our Drupal 8.8.x-dev build from using the Drupal Composer drupal-scaffold package https://github.com/drupal-composer/drupal-scaffold to using the Drupal 8 core composer scaffolding package (drupal/core-composer-scaffold).

I am trying to determine if I should submit a patch/merge request for changes to this core scaffolding package, because we are in need of additional configuration/functionality from it.

Problem is...

We have our web-root directory as ./www. The following is our composer-scaffold config in composer.json file:

...
      "composer-scaffold": {
          "allowed-packages": [
              "drupal/core"
          ],
          "locations": {
              "web-root": "./www"
          }
      },
...

Also, we keep our /vendor directory outside the web-root, not inside... ie. NOT ./www/vendor

When the composer scaffolding runs it generates the necessary autoload.php files in various directories. The one that is generated in our ./www directory looks like this:

./www/autoload.php

<?php

/**
 * @file
 * Includes the autoloader created by Composer.
 *
 * This file was generated by drupal-composer/drupal-scaffold.
 * https://github.com/drupal-composer/drupal-scaffold
 *
 * @see composer.json
 * @see index.php
 * @see core/install.php
 * @see core/rebuild.php
 * @see core/modules/statistics/statistics.php
 */

return require __DIR__ . '/./vendor/autoload.php';

The problem with this... is that it cannot find the /vendor/autoload.php file, because it does not exist at ./www/vendor/autoload.php. It does exist at ./vendor/autoload.php.

I did find a composer vendor-dir setting in the overall composer config vendor-dir. But, if I set it there... it messes up other composer stuff.

I was unable to find any vendor-dir setting in the composer scaffolding documentation or files themselves.

I see in the code itself that the function getVendorPath() in drupal/core-composer-scaffold/Handler.php is getting the vendor path from the composer config setting, as $vendor_dir = $this->composer->getConfig()->get('vendor-dir');

I am open to any advice on this. I am also able to code in contrib changes to allow an additional composer.json "locations" vendor-dir that could be used... if set. If not... it could default back to the overall composer config setting.

Example of new composer.json scaffolding config I could use:

...
      "composer-scaffold": {
          "allowed-packages": [
              "drupal/core"
          ],
          "locations": {
              "web-root": "./www",
              "vendor-dir": "../vendor"
          }
      },
...
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BinaryBlock created an issue. See original summary.

greg.1.anderson’s picture

Looks like you might have found a bug, @BinaryBlock. That autoload file should generate the correct path to your vendor directory without any configuration needed. If you do move your vendor directory with the `vendor-dir` configuration setting, then the scaffold plugin should still find the vendor directory wherever you put it.

We have an example repository https://github.com/greg-1-anderson/drupal-drupal-composer that you could take a look at.

I think that what's going on for you is that you probably committed your autoload.php file. If you do that, the scaffold plugin assumes that you know what you want, and does not overwrite the autoload file. So, try removing your autoload.php from git, run `composer composer:scaffold` again (or `composer install`), and see if a new, correct autoload.php file is generated.

danielnv18’s picture

I can confirm that this is an issue. I tested it with this repo greg-1-anderson/drupal-drupal-composer

it works if you want to output in the same directory. But if you want to use a sub directory the autoload.php doesn't have the correct path to the vendor directory.

For example using greg-1-anderson/drupal-drupal-composer, if you update the composer.json and add an extra level to the web-root/installer-paths

{
    "extra": {
        "composer-scaffold": {
            "allowed-packages": [
                "drupal/legacy-scaffold-assets",
                "drupal/core"
            ],
            "locations": {
                "web-root": "./dir1"
            }
        },
        "composer-exit-on-patch-failure": true,
        "patchLevel": {
            "drupal/core": "-p2"
        },
        "installer-paths": {
            "dir1/core": ["type:drupal-core"],
            "dir1/libraries/{$name}": ["type:drupal-library"],
            "dir1/modules/contrib/{$name}": ["type:drupal-module"],
            "dir1/profiles/contrib/{$name}": ["type:drupal-profile"],
            "dir1/themes/contrib/{$name}": ["type:drupal-theme"],
            "drush/Commands/{$name}": ["type:drupal-drush"]
        }
    }
}

the autoload points to the "same dir" instead of going up one level

<?php

/**
 * @file
 * Includes the autoloader created by Composer.
 *
 * This file was generated by composer-scaffold.
 *.
 * @see composer.json
 * @see index.php
 * @see core/install.php
 * @see core/rebuild.php
 * @see core/modules/statistics/statistics.php
 */

return require __DIR__ . '/./vendor/autoload.php';

greg.1.anderson’s picture

I think I've been testing these template files by seeing if `drush site:install` works with the layout; Drush does not care about the autoload.php file at the Drupal root, though.

I reproduced and found the problem. Filesystem::findShortestPath() should be called with the path to the autoload file, but it was using the parent directory of the autoload file instead.

Thanks for finding and reporting this bug.

greg.1.anderson’s picture

Title: Drupal core scaffolding : Add composer.json composer-scaffold "vendor-dir" config setting? » Drupal core scaffolding : path to vendor directory in generated autoload.php file is incorrect

Update title.

greg.1.anderson’s picture

Status: Active » Needs review
FileSize
6.16 KB

Here is a patch with a fix and a test. The provided test does fail on the 8.8.x branch, and passes with this fix.

greg.1.anderson’s picture

Issue summary: View changes
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

I can verify that the tests fail when you revert the changes to the component. It's fair to add a test-only patch in the issue... :-)

+++ b/core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ScaffoldTest.php
@@ -378,4 +380,27 @@ protected function assertCommonDrupalAssetsWereScaffolded($docroot, $is_link) {
+    $expected = "return require __DIR__ . '/vendor/autoload.php';";
+    if ($relocated_docroot) {
+      $expected = "return require __DIR__ . '/../vendor/autoload.php';";
+    }

I'd offer that this only really covers two cases for a much more arbitrary config, but it's not very likely that we'll see more than this, and if it breaks we can add more test cases as time progresses.

Other than that minor concern, RTBC.

Mixologic’s picture

+1 to RTBC from me.

@BinaryBlock, Thanks for such a detailed, thorough report. If you hadn't mentioned that this is your first issue, I would have assumed that you had been interacting with the queues for much longer.

@danielnv18 Thanks for the additional confirmation and info, its super helpful.

clemens.tolboom’s picture

Category: Feature request » Bug report
larowlan’s picture

The last submitted patch, 11: 3080649-11.fail_.patch, failed testing. View results

alexpott’s picture

+++ b/composer/Plugin/Scaffold/GenerateAutoloadReferenceFile.php
@@ -38,11 +38,12 @@ private function __construct() {
+    // Simplify the path if it's at the root
+    $relative_vendor_path = preg_replace('#^\./#', '', $relative_vendor_path);

I'm just wondering if we really need this. Hacking around with paths gives me concerns about Windows paths as we're hard-coding directory separators.

greg.1.anderson’s picture

@alexpott: That line just converts from '/./vendor/...' to '/vendor/'. This is a nicety that makes the generated file look better, but it does not change behavior at all. We could take out this line and be no worse off. Or, we can leave it in and not worry about whether it matches in Windows, because it won't hurt anything if it fails to apply. I'm actually not sure if the path separator here is going to be \ or / in this instance on Windows, but I suspect the later. I could investigate, if you thought it was important.

alexpott’s picture

So after a bit of discussion with @greg.1.anderson we realised the regex exists for \Drupal\Composer\Plugin\Scaffold\GenerateAutoloadReferenceFile::autoLoadContents() and so should live there along with the $vendor_path = rtrim($vendor_path, '/'); thats already manipulating the path. Keeping all the relative path tidying up in one place.

We also looked at \Composer\Util\Filesystem::findShortestPath() and decided that my concern in #13 about Windows paths is unfounded.

greg.1.anderson’s picture

I was able to get rid of the rtrim by calculating the relative path all the way to the autoload file rather than to the vendor directory; findShortestPath then normalizes for us.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a68ff42 and pushed to 8.8.x. Thanks!

diff --git a/composer/Plugin/Scaffold/GenerateAutoloadReferenceFile.php b/composer/Plugin/Scaffold/GenerateAutoloadReferenceFile.php
index c698a6b8fd..55e27debfe 100644
--- a/composer/Plugin/Scaffold/GenerateAutoloadReferenceFile.php
+++ b/composer/Plugin/Scaffold/GenerateAutoloadReferenceFile.php
@@ -90,8 +90,8 @@ protected static function autoloadPath($package_name, $web_root) {
   /**
    * Builds the contents of the autoload file.
    *
-   * @param string $vendor_path
-   *   The relative path to vendor.
+   * @param string $relative_autoload_path
+   *   The relative path to the autoloader in vendor.
    *
    * @return string
    *   Return the contents for the autoload.php.

Some docs updates :)

  • alexpott committed a68ff42 on 8.8.x
    Issue #3080649 by greg.1.anderson, larowlan, BinaryBlock, alexpott,...
BinaryBlock’s picture

WOW!!

I don't know what to say... but... Thank you to all who reviewed, researched, tested, coded, commented, etc. on this issue. I was really nervous and worried that my submission was going to be overlooked. I'm just so ecstatic this had so many eyes on it and was able to be resolved quickly.

Thank you for the core credit too! That's a big deal. This being my first post... it makes me feel so welcomed into the Drupal community.

I look forward to working with everyone in the future! :-)

larowlan’s picture

@BinaryBlock no - thank you - welcome to the community - in case you're not already there - pop in and say hi in the #contribute or #composer channel in slack and chat with the community in real time.

rachel_norfolk’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.