Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#53 | move_testing_packages-2444615-53.patch | 164.22 KB | jibran |
Comments
Comment #1
Fabianx CreditAttribution: Fabianx commentedComment #2
Fabianx CreditAttribution: Fabianx commentedComment #4
dawehnerIts certainly a good idea, especially when we want to get rid of the dependencies in GIT itself at some point.
Comment #5
larowlanWhat 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.
Comment #6
dawehnerWell, 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.
Comment #7
larowlanFYI I'm not arguing, this change is the right one, just pointing out that simpletest ui is the elephant in the room
Comment #8
daffie CreditAttribution: daffie commentedI 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.
Comment #9
jhedstromUpdated the title to reflect that mink and mink-goutte-driver should also be moved to require-dev.
Comment #10
jhedstromThis adds a
hook_requirements
check to simpletest, and also moves the behat mink packages torequire-dev
. Also attached is just a diff ofcomposer.json
andsimpletest.install
for easier review.Comment #12
jhedstromSo this would require changes to the testbot I think.
Comment #13
Mile23If you use
install --no-dev
then you shouldn't have PHPUnit in your classmap. If you doinstall --dev
(or justinstall
) 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, useinstall --dev
so that everyone can run tests as per @larowlan #7.Comment #14
larowlanWe should expand this to include symfony/console once #2493807: Add symfony/console component to core is in
Comment #15
davidwbarratt CreditAttribution: davidwbarratt as a volunteer and commentedFYI:
I mentioned this issue in my session at DrupalCon Los Angeles: Using Subtree Splits to spread Drupal into everything
Comment #16
catch+1.
Whenever I open up autoload_classes.php, the wall of this makes me sad:
Comment #17
larowlan@dawehner pointed out on irc we should add vfs to the require-dev list too. Similarly for symfony/console now that's in.
Comment #18
tim.plunkettComment #19
neclimdulTo 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
Comment #20
jhedstromThe 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.
Comment #21
neclimdulNot 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.
Comment #22
neclimdulok...?
Comment #23
dawehnerI 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?
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?
Comment #24
Crell CreditAttribution: Crell as a volunteer commentedI 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.
Comment #25
neclimdulHere's a re-roll for fun.
Comment #27
dawehnerNow that we are up to date, its really simple, just composer.json and composer.lock is changed.
Comment #28
klausican 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.
Comment #29
jibranYeah you are right @klausi updated the patch.
Comment #30
dawehnerGood point! We don't use it actively just indirect via mink.
Comment #31
jibranComment #32
klausican we sort this alphabetically?
Comment #33
dawehnerWell I prefer the order we have now, which adds a little bit of grouping, but sure, let's do it.
Comment #34
klausiLooks 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.
Comment #35
neclimduloh nice! yes! Good job guys.
Comment #36
Mile23RTBC +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
$
Comment #37
alexpottNeeds a reroll.
Comment #38
borisson_Reroll attached.
Comment #39
cilefen CreditAttribution: cilefen commenteddefault.settings.php snuck in
Comment #40
cilefen CreditAttribution: cilefen commentedIt looks like packages are being upgraded in this patch but should not be.
Comment #41
borisson_Fixes #39, sorry.
Comment #42
cilefen CreditAttribution: cilefen commentedCan one of the prior contributors post the command needed once the changes to composer.json are merged?
Comment #43
dawehnercomposer update behat/mink behat/mink-goutte-driver mikey179/vfsStream phpunit/phpunit
This is afaik all you need
Comment #44
borisson_I executed the command @dawehner posted in #43
Comment #45
cilefen CreditAttribution: cilefen commentedA
composer.lock
diff is impossible to read. Arg.Comment #46
Mile23No 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: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:
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.
Comment #47
jibranNW as catch said in #2546618-85: Document that we don't depend on the Symfony Translation component
Let's move it as well.
Comment #48
jibranComment #50
borisson_Reuploading the patch created in #48 by @jibran, I don't think these failures are related.
Comment #51
hussainwebRunning composer produces the same result. And it seems like tests have passed, so RTBC.
Comment #52
alexpott#2546618: Document that we don't depend on the Symfony Translation component landed ... needs yet another reroll - sorry.
Comment #53
jibranYAR
Comment #54
hussainwebcomposer update --lock
gives the same lock file, so all good...Comment #55
alexpottThis has been rerolled enough times. Committed 605264f and pushed to 8.0.x. Thanks!
Comment #57
dawehnerOh nice, I didn't saw that this actually landed. Thank you alex!