Marking as major because this means testing is broken for any project using composer (with or without composer_manager). No builds passing.

1) core/vendor/bin/phpunit includes the nearest autoloader, which is core/vendor/autoload.php
2) core/tests/bootstrap.php then blindly includes the root autoload.php
By default, the root autoload.php also points to core/vendor, and no crash occurs.

However, once you start using the root composer.json and have a root vendor directory, you overwrite autoload.php to point to root/vendor, and phpunit is now including a different autoloader. These two autoloaders conflict, and a crash occurs.

I've spoken to Fabianx about this and we agreed that there's no way to avoid the hack that is drupal_phpunit_get_extension_namespaces().
Instead, we should just use the phpunit that's in the root vendor directory ('./vendor/bin/phpunit -c core').
However, simpletest also invokes phpunit, and it hardcodes the one in core/vendor:

function simpletest_phpunit_command() {
  // Don't use the committed version in composer's bin dir if running on
  // windows.
  if (substr(PHP_OS, 0, 3) == 'WIN') {
    $php_executable_finder = new PhpExecutableFinder();
    $php = $php_executable_finder->find();
    $phpunit_bin = escapeshellarg($php) . " -f " . escapeshellarg(\Drupal::root() . "/core/vendor/phpunit/phpunit/composer/bin/phpunit") . " --";
  }
  else {
    $phpunit_bin = \Drupal::root() . "/core/vendor/bin/phpunit";
  }
  return $phpunit_bin;
}

Should we just add an is_dir(\Drupal::root() . '/vendor') and switch based on that? Or introduce an environment variable override?
Fabian suggested having an option to turn off phpunit invocation completely, but I don't like having to enforce two different testing workflows based on whether composer is used or not.

CommentFileSizeAuthor
#15 2554849-15.patch1.4 KBbojanz
#15 interdiff-14-15.txt1.37 KBbojanz
#14 2554849-14.patch1.41 KBhussainweb
#14 interdiff-9-14.txt1.46 KBhussainweb
#9 2554849.patch1.26 KBwebflo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bojanz created an issue. See original summary.

bojanz’s picture

bojanz CreditAttribution: bojanz commented

I should note that I've tried to have a different autoloader-prefix in the root composer.json, but that doesn't prevent the whole conflict because both autoloaders also try to unconditionally include core/lib/Drupal.php (cause it's in the "files" property in composer.json).

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented

I put my vendor directory in ../vendor so your proposed fix wouldn't help. I suggest we modify ./core/phpunit.dist.xml to exclude ../vendor or we have an example phpunit.xml in the root for people to use.

bojanz’s picture

bojanz CreditAttribution: bojanz commented

No phpunit.xml will help you avoid this issue, cause the second autoloader inclusion is coming from core/tests/bootstrap.php
It's not config controlled at all.

EDIT: To clarify, the main problem here is that run_tests.sh (simpletest) insists on invoking phpunit from a hardcoded location.
You are free to run phpunit from the root directory yourself, pointing it at core's config file, that works. But simpletest still fails.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

The first step is to avoid two vendor directories: #2380389: Use a single vendor directory in the root

Then we can just have phpunit.xml.dist at the root level.

Then it can find the right bootstrap file.

Then the bootstrapper can find the right autoload.

And all will be right with the world, and no one will have to waste time figuring out that they need to say ./vendor/bin/phpunit -c core

Example configurations which work around workarounds we can fix right now are a bad idea.

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented

Are you talking about:

$loader = require __DIR__ . '/../../autoload.php';

?

If so, the autoload.php file tells you to modify it:

This file can be edited to change the autoloader if you are managing a project's dependencies using Composer. If Drupal code requires the autoloader, it should always be loaded using this file so that projects using Composer continue to work.

which should prevent the second inclusion, no?

bojanz’s picture

bojanz CreditAttribution: bojanz commented

@Mile23
Please stop repeating the same only slightly relevant story on each issue.

@davidwbarratt
No. I wrote in the issue summary clearly where the two inclusions happen:
core/vendor/bin/phpunit:

foreach (array(__DIR__ . '/../../autoload.php', __DIR__ . '/../vendor/autoload.php', __DIR__ . '/vendor/autoload.php') as $file) {
    if (file_exists($file)) {
        define('PHPUNIT_COMPOSER_INSTALL', $file);
        break;
    }
}
require PHPUNIT_COMPOSER_INSTALL;

core/tests/bootstrap.php:

function drupal_phpunit_get_extension_namespaces($dirs) {
// Start with classes in known locations.
$loader = require __DIR__ . '/../../autoload.php';

The autoload.php is the root file you are referring to, the one now pointing to the root vendor dir, therefore causing the crash.

davidwbarratt’s picture

davidwbarratt CreditAttribution: davidwbarratt at Golf Channel commented

Ah! I understand now. sorry, I'm not sure why I was so confused.

I got around this by doing what you recommended:

Instead, we should just use the phpunit that's in the root vendor directory ('./vendor/bin/phpunit -c core').

I do that mostly because I just pretend that ./core/vendor doesn't exist.

However, I haven't run simpletest, but looking at what you posted, yes it would break. :/

But also, despite Mile23's enthusiasm, #2380389: Use a single vendor directory in the root doesn't not solve the problem if you are like me and your vendor directory is not ./vendor but is instead ../vendor (or anywhere else for that matter).

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2554849.patch1.26 KB

AFAIK we just have to remove the hardcoded path and retrieve the path from autoload.php.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Active » Needs work

@Mile23
Please stop repeating the same only slightly relevant story on each issue.

This is precisely why I'm cynical about anyone finding a solution: No one will listen to the answer when its handed to them.

It's the same story because it's the same problem: If we put everything in *1* place, then we can modify run-tests.sh to look in the *1* place.

But what the hell do I know?

But also, despite Mile23's enthusiasm, #2380389: Use a single vendor directory in the root doesn't not solve the problem if you are like me and your vendor directory is not ./vendor but is instead ../vendor (or anywhere else for that matter).

Wrong.

If we normalize on a single vendor directory, then changes to where that vendor directory is located become easier. If Drupal knows that there is only one vendor directory, then you can fake it out more easily.

If you solve this issue by making another workaround, you make it increasingly more impossible to solve the real problem.

BTW, before I realized I was not welcome to help solve this issue, I made a travis test. Go ahead and fork it if it doesn't actually reflect the behavior you're discussing. It passes right now because it's broken.

https://travis-ci.org/paul-m/d8-drupal-require-mod-autoload

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

#9 works for me. I think it's as clean as we can get. Thank you, webflo.

+1 to RTBC.

EDIT: We could use a comment reminding the reader why we're using reflection. I can do that reroll.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Issue tags: +Composer
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
FileSize
interdiff-9-14.txt1.46 KB
2554849-14.patch1.41 KB

I added a comment to explain why we are using reflection. I also changed the variable name to be something hopefully more descriptive.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
FileSize
interdiff-14-15.txt1.37 KB
2554849-15.patch1.4 KB

You beat me to it :)

Converted it's to its, and double quotes to single quotes, since we're changing those lines anyway and this is more in line with what core usually does.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano at Druid commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/simpletest/simpletest.module
@@ -287,15 +287,21 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
+  // reflection. We can determine the vendor directory based on that filename.

I'd extend this comment with an explanation of how/why we can determine the dir, but that's a nitpick and isn't in the way of RTBC.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

Good find!

RTBC +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed f82e0a3 and pushed to 8.0.x. Thanks!

  • alexpott committed f82e0a3 on 8.0.x 
    Issue #2554849 by bojanz, hussainweb, webflo: phpunit testing crashes if...

Status: Fixed » Closed (fixed)

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