Problem/Motivation

Following packages are only needed for testing:

    "phpunit/phpunit": "4.8.*",
    "mikey179/vfsStream": "~1.2",
    "behat/mink": "~1.6",
    "behat/mink-goutte-driver": "dev-master#cc5ce119b5a8e06662f634b35967aff0b0c7dfdd"

Proposed resolution

Move them under "required-dev" key in composer.json

Remaining tasks

Review.
Commit

User interface changes

None

API changes

None

Data model changes

None

Original report by @Fabianx

I was able to get composer to move phpunit to require-dev without having to composer update.

A quick test of:

rm -rf vendor/
composer install

lead to the same code checkout except some modes and CLRF, which are unrelated to this change.

Possible to do now theoretically:

$ composer install --no-dev # removes phpunit from classmap
$ git add vendor/composer/autoload*
$ composer install
$ git checkout vendor/composer/autoload*

to have a cleaned up classmap with phpunit installed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: move_phpunit_to-2444615-1.patch, failed testing.

dawehner’s picture

Its certainly a good idea, especially when we want to get rid of the dependencies in GIT itself at some point.

larowlan’s picture

What if you enable the simpletest UI and want to run phpunit tests from there (yes you're mental but it can be done)?

In that case I think we should have simpletest add the phpunit namespace to the autoloader at runtime.

Same for the mink/goutte stuff.

dawehner’s picture

What if you enable the simpletest UI and want to run phpunit tests from there (yes you're mental but it can be done)?

Well, I'd suggest to use hook_requirements to block simpletest from being installed in case phpunit isn't there, but for most deployments
you would never ever need phpunit itself.

larowlan’s picture

FYI I'm not arguing, this change is the right one, just pointing out that simpletest ui is the elephant in the room

daffie’s picture

Component: other » phpunit

I am not sure what exactly the problem is. But we are working with PHPUnit version 4.4 in HEAD. The current version PHPUnit is a the moment 4.5.

jhedstrom’s picture

Title: Move phpunit to require-dev » Move phpunit and mink to require-dev

Updated the title to reflect that mink and mink-goutte-driver should also be moved to require-dev.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
330.09 KB
1.43 KB

This adds a hook_requirements check to simpletest, and also moves the behat mink packages to require-dev. Also attached is just a diff of composer.json and simpletest.install for easier review.

Status: Needs review » Needs work

The last submitted patch, 10: phpunit-require-dev-2444615-10.patch, failed testing.

jhedstrom’s picture

So this would require changes to the testbot I think.

Mile23’s picture

If you use install --no-dev then you shouldn't have PHPUnit in your classmap. If you do install --dev (or just install) then you *should* have PHPUnit in your classmap, because that's how it works. :-)

If PHPUnit should use PSR-4 or whatever then file that issue upstream.

+1 on moving non-essential stuff to require-dev. The Drupal packager must, however, use install --dev so that everyone can run tests as per @larowlan #7.

larowlan’s picture

We should expand this to include symfony/console once #2493807: Add symfony/console component to core is in

davidwbarratt’s picture

FYI:

I mentioned this issue in my session at DrupalCon Los Angeles: Using Subtree Splits to spread Drupal into everything

catch’s picture

+1.

Whenever I open up autoload_classes.php, the wall of this makes me sad:

  'File_Iterator' => $vendorDir . '/phpunit/php-file-iterator/src/Iterator.php',
    'File_Iterator_Facade' => $vendorDir . '/phpunit/php-file-iterator/src/Facade.php',
    'File_Iterator_Factory' => $vendorDir . '/phpunit/php-file-iterator/src/Factory.php',
    'PHPUnit_Exception' => $vendorDir . '/phpunit/phpunit/src/Exception.php',
    'PHPUnit_Extensions_GroupTestSuite' => $vendorDir . '/phpunit/phpunit/src/Extensions/GroupTestSuite.php',
    'PHPUnit_Extensions_PhptTestCase' => $vendorDir . '/phpunit/phpunit/src/Extensions/PhptTestCase.php',
    'PHPUnit_Extensions_PhptTestSuite' => $vendorDir . '/phpunit/phpunit/src/Extensions/PhptTestSuite.php',
    'PHPUnit_Extensions_RepeatedTest' => $vendorDir . '/phpunit/phpunit/src/Extensions/RepeatedTest.php',
    'PHPUnit_Extensions_TestDecorator' => $vendorDir . '/phpunit/phpunit/src/Extensions/TestDecorator.php',
    'PHPUnit_Extensions_TicketListener' => $vendorDir . '/phpunit/phpunit/src/Extensions/TicketListener.php',
    'PHPUnit_Framework_Assert' => $vendorDir . '/phpunit/phpunit/src/Framework/Assert.php',
    'PHPUnit_Framework_AssertionFailedError' => $vendorDir . '/phpunit/phpunit/src/Framework/AssertionFailedError.php',
    'PHPUnit_Framework_BaseTestListener' => $vendorDir . '/phpunit/phpunit/src/Framework/BaseTestListener.php',
    'PHPUnit_Framework_CodeCoverageException' => $vendorDir . '/phpunit/phpunit/src/Framework/CodeCoverageException.php',
    'PHPUnit_Framework_Constraint' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint.php',
    'PHPUnit_Framework_Constraint_And' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/And.php',
    'PHPUnit_Framework_Constraint_ArrayHasKey' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ArrayHasKey.php',
    'PHPUnit_Framework_Constraint_ArraySubset' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ArraySubset.php',
    'PHPUnit_Framework_Constraint_Attribute' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Attribute.php',
    'PHPUnit_Framework_Constraint_Callback' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Callback.php',
    'PHPUnit_Framework_Constraint_ClassHasAttribute' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ClassHasAttribute.php',
    'PHPUnit_Framework_Constraint_ClassHasStaticAttribute' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ClassHasStaticAttribute.php',
    'PHPUnit_Framework_Constraint_Composite' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Composite.php',
    'PHPUnit_Framework_Constraint_Count' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Count.php',
    'PHPUnit_Framework_Constraint_Exception' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Exception.php',
    'PHPUnit_Framework_Constraint_ExceptionCode' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ExceptionCode.php',
    'PHPUnit_Framework_Constraint_ExceptionMessage' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ExceptionMessage.php',
    'PHPUnit_Framework_Constraint_ExceptionMessageRegExp' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ExceptionMessageRegExp.php',
    'PHPUnit_Framework_Constraint_FileExists' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/FileExists.php',
    'PHPUnit_Framework_Constraint_GreaterThan' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/GreaterThan.php',
    'PHPUnit_Framework_Constraint_IsAnything' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsAnything.php',
    'PHPUnit_Framework_Constraint_IsEmpty' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsEmpty.php',
    'PHPUnit_Framework_Constraint_IsEqual' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsEqual.php',
    'PHPUnit_Framework_Constraint_IsFalse' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsFalse.php',
    'PHPUnit_Framework_Constraint_IsIdentical' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php',
    'PHPUnit_Framework_Constraint_IsInstanceOf' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsInstanceOf.php',
    'PHPUnit_Framework_Constraint_IsJson' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsJson.php',
    'PHPUnit_Framework_Constraint_IsNull' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsNull.php',
    'PHPUnit_Framework_Constraint_IsTrue' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsTrue.php',
    'PHPUnit_Framework_Constraint_IsType' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/IsType.php',
    'PHPUnit_Framework_Constraint_JsonMatches' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/JsonMatches.php',
    'PHPUnit_Framework_Constraint_JsonMatches_ErrorMessageProvider' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/JsonMatches/ErrorMessageProvider.php',
    'PHPUnit_Framework_Constraint_LessThan' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/LessThan.php',
    'PHPUnit_Framework_Constraint_Not' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Not.php',
    'PHPUnit_Framework_Constraint_ObjectHasAttribute' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/ObjectHasAttribute.php',
    'PHPUnit_Framework_Constraint_Or' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/Or.php',
    'PHPUnit_Framework_Constraint_PCREMatch' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/PCREMatch.php',
    'PHPUnit_Framework_Constraint_SameSize' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/SameSize.php',
    'PHPUnit_Framework_Constraint_StringContains' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/StringContains.php',
    'PHPUnit_Framework_Constraint_StringEndsWith' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/StringEndsWith.php',
    'PHPUnit_Framework_Constraint_StringMatches' => $vendorDir . '/phpunit/phpunit/src/Framework/Constraint/StringMatches.php',

larowlan’s picture

@dawehner pointed out on irc we should add vfs to the require-dev list too. Similarly for symfony/console now that's in.

tim.plunkett’s picture

Title: Move phpunit and mink to require-dev » Move phpunit, mink, vfs, and symfony/console to require-dev
neclimdul’s picture

Status: Needs work » Needs review
FileSize
5.45 MB
154.91 KB

To change catch's map file note I think we have to remove dev from core. I'm actually ok with that but it'll require some infrastructure changes(testbot running composer.phar stuff)

For the patch including require-dev I did
composer.phar update --lock

and for the no-dev patch I did this. Man I really wish we didn't keep our dependencies in git... And yeah you're reading that right. 5mb of mostly removals.
composer.phar update --no-dev symfony/console mikey179/vfsstream phpunit/phpunit phpunit/php-code-coverage phpunit/php-file-iterator phpunit/php-token-stream phpunit/phpunit-mock-objects phpspec/prophecy phpdocumentor/reflection-docblock sebastian/comparator sebastian/diff sebastian/environment sebastian/exporter sebastian/recursion-context sebastian/version behat/mink-goutte-driver behat/mink-browserkit-driver behat/mink sebastian/global-state phpunit/php-timer doctrine/instantiator phpunit/php-text-template symfony/translation

jhedstrom’s picture

The approach in #19 differs from the original approach here in that it fully removes the libraries. The previous patches only removed them from the classmap. I think fully removing them makes sense, since the classmap would have to be updated locally either way, but the issue summary should be updated accordingly.

neclimdul’s picture

Status: Needs review » Needs work

Not 100% true, I provided 2 patches to demonstrate the effect of solving #16 and allow for discussion if needed. The -dev patch that went to testbot is the same approach and just a reroll.

neclimdul’s picture

Status: Needs work » Needs review

ok...?

dawehner’s picture

I really like to separate the requirements. I'm wondering whether we could in simpletest add a hook_requirements() for those libraries because otherwise, you get failures when running the tests in some potential future, when the libraries aren't committed anymore?

+++ b/core/composer.json
@@ -25,16 +24,19 @@
   },
+  "require-dev": {
...
+    "symfony/console": "2.7.*",

I'm not entirely sure that coming symfony/console to dev is the right idea? A couple of actual user scripts will potentially rely on it, so it should better exist always?

Crell’s picture

I would prefer to keep Console in non-dev as well. Even if core doesn't use it for non-dev commands, I don't see why we shouldn't allow core to do so easily.

Until we have composer fully working, with all of vendor removed from the repository, I'm skeptical about removing the dev dependencies. That would require developers to rerun composer install, but from all the rerolls we've done that seems to sometimes cause issues and break things with other libs. (I don't know why, since it's not supposed to, but I see plenty of buggy patch lines every time we try to upgrade a dependency.) I worry we'll be causing as many problems as we solve.

neclimdul’s picture

Here's a re-roll for fun.

Status: Needs review » Needs work

The last submitted patch, 25: require_dev-vendor_changes.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
157.34 KB

Now that we are up to date, its really simple, just composer.json and composer.lock is changed.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/composer.json
@@ -24,18 +24,20 @@
+  "require-dev": {
+    "phpunit/phpunit": "4.8.*",
+    "mikey179/vfsStream": "~1.2",
+    "behat/mink": "~1.6",
+    "behat/mink-goutte-driver": "dev-master#cc5ce119b5a8e06662f634b35967aff0b0c7dfdd"
+  },

can we order this alphabetically please?

Why is Goutte still in "require"? I don't see it being used in core except for BrowserTestBase.php, so it should also be in "require-dev"? IMO we should remove fabpot/goutte completely, that is added by behat/mink-goutte-driver automatically as dependency anyway.

jibran’s picture

Yeah you are right @klausi updated the patch.

dawehner’s picture

Why is Goutte still in "require"? I don't see it being used in core except for BrowserTestBase.php, so it should also be in "require-dev"? IMO we should remove fabpot/goutte completely, that is added by behat/mink-goutte-driver automatically as dependency anyway.

Good point! We don't use it actively just indirect via mink.

jibran’s picture

Title: Move phpunit, mink, vfs, and symfony/console to require-dev » Move testing packages to require-dev in composer.json
Issue tags: -Needs issue summary update
klausi’s picture

Status: Needs review » Needs work
+++ b/core/composer.json
@@ -24,18 +24,19 @@
+  "require-dev": {
+    "phpunit/phpunit": "4.8.*",
+    "mikey179/vfsStream": "~1.2",
+    "behat/mink": "~1.6",
+    "behat/mink-goutte-driver": "dev-master#cc5ce119b5a8e06662f634b35967aff0b0c7dfdd"
+  },

can we sort this alphabetically?

dawehner’s picture

Status: Needs work » Needs review
FileSize
164.02 KB
1.08 KB

Well I prefer the order we have now, which adds a little bit of grouping, but sure, let's do it.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, verified that the outcome of composer update is the same on my local machine.

RTBC since this only touches composer json files which cannot be tested by the testbot.

neclimdul’s picture

oh nice! yes! Good job guys.

Mile23’s picture

RTBC +1.

$ git apply 2444615-33.patch
$ cd core
$ composer install --dev
You are using the deprecated option "dev". Dev packages are installed by default now.
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess
$ git diff --name-only
core/composer.json
core/composer.lock
$

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

borisson_’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
157.48 KB

Reroll attached.

cilefen’s picture

Status: Needs review » Needs work
+++ b/core/composer.lock
--- a/sites/default/default.settings.php
+++ b/sites/default/default.settings.php

+++ b/sites/default/default.settings.php
+++ b/sites/default/default.settings.php
@@ -238,6 +238,7 @@

@@ -238,6 +238,7 @@
  * Example:
  * @code
  *   $config_directories = array(
+ *     CONFIG_ACTIVE_DIRECTORY => '/some/directory/outside/webroot',
  *     CONFIG_STAGING_DIRECTORY => '/another/directory/outside/webroot',
  *   );
  * @endcode

default.settings.php snuck in

cilefen’s picture

+++ b/core/composer.lock
@@ -1577,39 +1368,56 @@
+            "version": "v2.7.4",

It looks like packages are being upgraded in this patch but should not be.

borisson_’s picture

Status: Needs work » Needs review
FileSize
157.05 KB

Fixes #39, sorry.

cilefen’s picture

Can one of the prior contributors post the command needed once the changes to composer.json are merged?

dawehner’s picture

composer update behat/mink behat/mink-goutte-driver mikey179/vfsStream phpunit/phpunit
This is afaik all you need

borisson_’s picture

I executed the command @dawehner posted in #43

cilefen’s picture

A composer.lockdiff is impossible to read. Arg.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Can one of the prior contributors post the command needed once the changes to composer.json are merged?

No commands are needed. They're the same libraries in core/vendor.

You can, however, now say composer install --no-dev and the testing stuff will be magically removed, like this:

$ cd core
$ composer install --no-dev
Loading composer repositories with package information
Installing dependencies from lock file
  - Removing sebastian/version (1.0.6)
  - Removing sebastian/recursion-context (1.0.1)
  - Removing sebastian/exporter (1.2.1)
  - Removing sebastian/comparator (1.2.0)
  - Removing phpunit/phpunit-mock-objects (2.3.7)
  - Removing phpunit/php-token-stream (1.4.6)
  - Removing phpunit/php-text-template (1.2.1)
  - Removing phpunit/php-file-iterator (1.4.1)
  - Removing phpspec/prophecy (v1.5.0)
  - Removing phpunit/phpunit (4.8.6)
  - Removing phpunit/php-code-coverage (2.2.2)
  - Removing phpunit/php-timer (1.0.7)
  - Removing sebastian/environment (1.3.2)
  - Removing doctrine/instantiator (1.0.5)
  - Removing behat/mink-goutte-driver (dev-master cc5ce11)
  - Removing mikey179/vfsstream (v1.5.0)
  - Removing phpdocumentor/reflection-docblock (2.0.4)
  - Removing sebastian/diff (1.3.0)
  - Removing behat/mink-browserkit-driver (v1.2.0)
  - Removing behat/mink (v1.6.1)
  - Removing sebastian/global-state (1.0.0)
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess
$

Want to run tests? It's time to composer install again which in my case loaded all the libraries from caches. (You can add --dev to be explicit, but it's the default now.)

When we do that, we end up with:

$ git add core/
$ git diff --name-only --cached
core/composer.json
core/composer.lock
core/vendor/composer/installed.json
core/vendor/phpunit/phpunit/tests/Regression/GitHub/873-php5.phpt
core/vendor/phpunit/phpunit/tests/Regression/GitHub/873-php7.phpt
core/vendor/phpunit/phpunit/tests/Regression/GitHub/873.phpt
core/vendor/phpunit/phpunit/tests/Regression/GitHub/873.phpt.orig
core/vendor/phpunit/phpunit/tests/Regression/GitHub/873.phpt.rej
core/vendor/phpunit/phpunit/tests/_files/Error.php
core/vendor/phpunit/phpunit/tests/_files/TestTestError.php

I'm RTBCing because this should be an option and this is as close as we can get to a usable patch, given our process.

jibran’s picture

Status: Reviewed & tested by the community » Needs work

NW as catch said in #2546618-85: Document that we don't depend on the Symfony Translation component

Isn't +symfony/css-selector also dev-only?

Let's move it as well.

jibran’s picture

Status: Needs work » Needs review
FileSize
160.83 KB
  "require-dev": {
    "behat/mink": "~1.6",
    "behat/mink-goutte-driver": "dev-master#cc5ce119b5a8e06662f634b35967aff0b0c7dfdd",
    "mikey179/vfsStream": "~1.2",
    "phpunit/phpunit": "4.8.*",
    "symfony/css-selector": "2.7.*"
  },

Status: Needs review » Needs work

The last submitted patch, 48: move_testing_packages-2444615-48.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
160.83 KB

Reuploading the patch created in #48 by @jibran, I don't think these failures are related.

hussainweb’s picture

Status: Needs review » Reviewed & tested by the community

Running composer produces the same result. And it seems like tests have passed, so RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
jibran’s picture

Status: Needs work » Needs review
FileSize
164.22 KB

YAR

hussainweb’s picture

Status: Needs review » Reviewed & tested by the community

composer update --lock gives the same lock file, so all good...

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This has been rerolled enough times. Committed 605264f and pushed to 8.0.x. Thanks!

  • alexpott committed 605264f on 8.0.x
    Issue #2444615 by borisson_, neclimdul, jibran, dawehner, jhedstrom,...
dawehner’s picture

Oh nice, I didn't saw that this actually landed. Thank you alex!

Status: Fixed » Closed (fixed)

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