Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Drupal 8 vendor folder outside webroot #12499

Merged
merged 1 commit into from
Jul 18, 2018

Conversation

wannesderoy
Copy link
Contributor

Overview

Allow Drupal 8 vendor folder outside webroot

Before

Drupal dependencies in vendor folder could not be located outside webroot. Autoloading does not work.

After

Drupal dependencies in vendor folder can not be located outside webroot. Autoloading works.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot
Copy link

civibot bot commented Jul 17, 2018

(Standard links)

@eileenmcnaughton
Copy link
Contributor

This is trying to fix the same #12198

@dsnopek the above seems to have stalled on a minor style issue - seems non-blocking but also easily fixed

@dsnopek
Copy link
Contributor

dsnopek commented Jul 17, 2018

Assuming we can guaranty that composer always generates this autoload.php in the web root, then this change is fine - even better than my earlier PR :-)

The thing is, while I can confirm that I see an autoload.php in all my Drupal projects, I don't see anything in the composer docs about it:

https://getcomposer.org/doc/01-basic-usage.md#autoloading

... and I don't see that in my non-Drupal composer-based projects. So, it must be some Drupal thing that generates it? If we can pin down what that is, and know that it'll always be there, then this is fine.

@eileenmcnaughton
Copy link
Contributor

@dsnopek what about the cautious approach - try webroot first but fall back (possibly with some sort of notice) to vendor?

@eileenmcnaughton
Copy link
Contributor

add to whitelist

@wannesderoy
Copy link
Contributor Author

As far as I know the autoload.php is always included in the Drupal 8 webroot. It isn't even generated by composer.
The autoload.php is required in de index.php of Drupal 8, so a project without it will not work. I don't even see the necessity of a cautious approach like @eileenmcnaughton suggests. no autoload.php in de webroot = no site :)

@eileenmcnaughton
Copy link
Contributor

@dsnopek @wannesderoy I think this link https://api.drupal.org/api/drupal/autoload.php/8.0.x might an indication that the file is in all branches

@eileenmcnaughton
Copy link
Contributor

test this please

@monishdeb
Copy link
Member

monishdeb commented Jul 18, 2018

Hmm after reviewing @dsnopek and this PR, and also considering the fact that it's not always true to have autoload.php under webroot but in some cases it would be under webroot/vendor/ as per https://api.drupal.org/api/drupal/autoload.php/8.0.x I would suggest the following change:

$autoloaderPath = array_filter([$root . '/autoload.php', $root . '/vendor/autoload.php'], function($path) {
  if (file_exists($path)) {
    return $path;
  }
})[0];
$autoloader = require_once $autoloaderPath;
...

^^@eileenmcnaughton

@eileenmcnaughton
Copy link
Contributor

@monishdeb

also considering the fact that it's not always true to have autoload.php under webroot

  • can you provide some link / evidence of this? @wannesderoy is of the feeling it IS but it may not always be under webroot/vendor

@monishdeb
Copy link
Member

can you provide some link / evidence of this? @wannesderoy is of the feeling it IS but it may not always be under webroot/vendor

After some detail investigation, I looked upon Drupal branches, checked the directory structure of composer create-project drupal-composer/drupal-project:8.x-dev and the webroot always got autload.php which has the following content:

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

OR

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

So I agree with @wannesderoy to rely on autload.php which includes the vendor/autoload.php inside or outside the webroot based on the different approach to create Drupal instance.

@dsnopek are you ok with merging this PR?

@wannesderoy
Copy link
Contributor Author

@monishdeb Exactly! The autoload.php in the root always has the correct path to the vendor folder. So to me it seems this is the best/simplest solution. No need for checks and fallbacks.
Thanks for confirming and explaining it so clear.

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Jul 18, 2018
@eileenmcnaughton
Copy link
Contributor

Ok - I think this is merge-ready - ie. if we hear a confirmation from @dsnopek or no-one comments for a few days & tests pass it can be merged & the related PR can be closed

@dsnopek
Copy link
Contributor

dsnopek commented Jul 18, 2018

I was hoping that we could first at least find the code that's generating that extra autoload.php. That would increase my confidence in it!

@dsnopek
Copy link
Contributor

dsnopek commented Jul 18, 2018

Ok, so I dug into this a bit. It looks like the autoload.php in the release tarballs on Drupal.org was written by hand. It was added in this issue:

https://www.drupal.org/node/2406681

When using composer create-project drupal-composer/drupal-project it gets generated by the 'drupal-composer/drupal-scaffold' composer plugin.

Looking at the original D.o issue, it seems unlikely that this file will get removed - the idea was to encourage people to use it rather than vendor/autoload.php to allow moving the vendor directory. I suppose the technique for doing this could change in the Drupal 8 composer initiative, but I doubt that'll be removed in Drupal 8 to maintain BC (maybe in Drupal 9).

In short: 👍 to this PR :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections sig:compatibility drupal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants