Reproduce:

$ cd ../your_drupal8_folder/core/
$ drush status
[...]
Drupal root            :  ../your_drupal8_folder/core/

The reason is a bug in drush_valid_drupal_root. Patch is coming up in a few minutes.

CommentFileSizeAuthor
#15 drush-wrong-drupal-root-1874726-15.patch665 byteshansfn
#9 drush-wrong-drupal-root-1874726-9.patch726 byteshansfn
#7 drush-wrong-drupal-root-1874726-7.patch1.04 KBhansfn
#6 drush-1874726.patch1.03 KBjonhattan
#3 drush-wrong-drupal-root-1874726-3.patch1.08 KBhansfn
#2 drush-wrong-drupal-root-1874726-2.patch1.08 KBhansfn
#1 drush-wrong-drupal-root-1874726.patch1.09 KBhansfn

Comments

hansfn’s picture

hansfn commented
Status: Active » Needs review
StatusFileSize
new drush-wrong-drupal-root-1874726.patch1.09 KB
hansfn’s picture

hansfn commented
StatusFileSize
new drush-wrong-drupal-root-1874726-2.patch1.08 KB

Tabs replaced with spaces ;-)

hansfn’s picture

hansfn commented
StatusFileSize
new drush-wrong-drupal-root-1874726-3.patch1.08 KB

Removed a typo - drupal7 and drupal8 key mixed up in last return statement.

moshe weitzman’s picture

moshe weitzman commented
Version: » 8.x-6.x-dev
Status: Needs review » Needs work

Code still looks confused about version. Maybe it is deliberate but it looks weird at quick lance. See these two adjacent lines:

+    elseif (file_exists($path . '/' . $candidates['drupal7'])) {
+      // Check if we are somewhere inside the 'core' directory of a Drupal 8 installation
hansfn’s picture

hansfn commented

Thx for taking a look. The code isn't confused at all. Maybe an example will explain what I'm trying to handle:

You are located inside /docroot/drupal8/core (or any subdirectory) when running drush. From that location, the Drupal 7 path candidate "includes/common.inc" will match. Without an extra check to see if we are inside a D8 core directory, Drush will accept this as the Drupal root and we get "/docroot/drupal8/core" as root in stead of the correct "/docroot/drupal8".

The patch fixes this problem, without breaking the existing behavior. Maybe my comment could be clearer - it was the best I could come up with. Accept for this, I don't see that the patch needs more work ;-)

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Component: Config » Base system (internal API)
Status: Needs work » Needs review
StatusFileSize
new drush-1874726.patch1.03 KB

Proposed resolution

hansfn’s picture

hansfn commented
StatusFileSize
new drush-wrong-drupal-root-1874726-7.patch1.04 KB

The patch above is functionally equivalent to mine. I guess the "../sites/sites.php" extra check is to catch the case when someone installs Drupal 7 in a folder named "core"?

Some issues:
1) The comment says "core/sites/sites.php" which doesn't exist AFAIK.
2) In a "standard" installation "../sites/sites.php" also doesn't exist so checking for it doesn't add any value. Maybe check for "../sites/example.sites.php" (or "../sites/default") instead.

Updated patch attached.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Status: Needs review » Fixed

You're right. Committed #7

hansfn’s picture

hansfn commented
StatusFileSize
new drush-wrong-drupal-root-1874726-9.patch726 bytes

Hm, I just tested and there is a tiny bug in our patch - a missing slash. See attached patch.

Another minor thing: In the D7 case we added an extra check in case a user installed D7 inside a directory named "core". If we want to be really pedantic (and consistent), I guess we should add the same check in the D8 case: If we are running drush from the parent directory of a D7 installation in a directory named "core", the current code will return wrong Drupal root since the D8 candidate matches.

hansfn’s picture

hansfn commented
Status: Fixed » Active

Hm, maybe I should have set the status to active ;-)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Reviewed & tested by the community

Works for me, thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Active

I missed #8, I was RTBCing a committed patch :)

hansfn’s picture

hansfn commented
Status: Active » Fixed

It seems my trivial bug fix in comment #9 has been committed (by moshe weitzman on the 29th of January). Apparently your RTBC had some effect ;-)

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

I had a more accurate patch locally to check for the presence of composer.json. Committed: http://drupalcode.org/project/drush.git/commit/a6fedd1

hansfn’s picture

hansfn commented
Status: Fixed » Active
StatusFileSize
new drush-wrong-drupal-root-1874726-15.patch665 bytes

Nice, jonhattan. There is one minor issue with your patch - your replace and with or in the "Drupal 7 root" case. This means that a user installing Drupal 7 in a directory named "core" is in trouble ... I have attached a trivial fix.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

@hansfn #14 works for me. Did you test it?

(file_exists($path . '/' . $candidate) && ((basename($path) != 'core') || !file_exists($path . '/../core/composer.json')))

As I read it: candidate exists AND basename is not core OR (because basename is core) core/composer.json doesn't exist.

hansfn’s picture

hansfn commented
Status: Active » Fixed

Sorry, jonhattan, your code works - I was wrong. I misunderstood the logic.

PS! If basename($path) is 'core', you might use
!file_exists($path . '/composer.json')
in stead of
!file_exists($path . '/../core/composer.json')

Status: Fixed » Closed (fixed)

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

BMDan’s picture

BMDan commented
Status: Active » Closed (fixed)

composer.json is no longer in the root of core, so being anywhere under "core" results in misidentification of "core/" as a D7 root.

Full list of "composer.json"s under the real docroot:

# find . -name composer.json
./core/vendor/kriswallsmith/assetic/composer.json
./core/vendor/phpunit/phpunit/composer.json
./core/vendor/phpunit/php-code-coverage/composer.json
./core/vendor/phpunit/php-text-template/composer.json
./core/vendor/phpunit/php-file-iterator/composer.json
./core/vendor/phpunit/php-timer/composer.json
./core/vendor/phpunit/php-token-stream/composer.json
./core/vendor/phpunit/phpunit-mock-objects/composer.json
./core/vendor/symfony/validator/Symfony/Component/Validator/composer.json
./core/vendor/symfony/serializer/Symfony/Component/Serializer/composer.json
./core/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/composer.json
./core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/composer.json
./core/vendor/symfony/routing/Symfony/Component/Routing/composer.json
./core/vendor/symfony/class-loader/Symfony/Component/ClassLoader/composer.json
./core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/composer.json
./core/vendor/symfony/yaml/Symfony/Component/Yaml/composer.json
./core/vendor/symfony/process/Symfony/Component/Process/composer.json
./core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/composer.json
./core/vendor/symfony-cmf/routing/Symfony/Cmf/Component/Routing/composer.json
./core/vendor/guzzle/common/Guzzle/Common/composer.json
./core/vendor/guzzle/stream/Guzzle/Stream/composer.json
./core/vendor/guzzle/http/Guzzle/Http/composer.json
./core/vendor/guzzle/parser/Guzzle/Parser/composer.json
./core/vendor/easyrdf/easyrdf/composer.json
./core/vendor/twig/twig/composer.json
./core/vendor/psr/log/composer.json
./core/vendor/doctrine/common/composer.json
./composer.json

Based on this, it looks like the test should be ../composer.json instead of ../core/composer.json, but I'm hesitant to actually suggest that without understanding the full logic.

BMDan’s picture

BMDan commented
Status: Closed (fixed) » Active
hansfn’s picture

hansfn commented
Status: Closed (fixed) » Active

Based on this, it looks like the test should be ../composer.json instead of ../core/composer.json, but I'm hesitant to actually suggest that without understanding the full logic.

That is the correct fix AFAICT too. The logic is simple - there is no composer.json in D7 (or D6) so the existence of such a file indicates very strongly that this is D8 ;-)

moshe weitzman’s picture

moshe weitzman commented
Status: Active » Fixed

Committed as suggested. Thanks BMDan and hansfn

Status: Fixed » Closed (fixed)

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