Problem/Motivation
We want d.o to ship a tarball without dev dependencies, for a variety of reasons including security. This means we have to change the build process so it uses composer install --no-dev
: #2745355: Use "composer install --no-dev" to create tagged core packages
However, the simpletest module has undeclared dependencies on the phpunit/phpunit package. If we ship a --no-dev
tarball, simpletest module will crash when users enable it and visit admin/config/development/testing.
We have an issue for disallowing extensions from enabling if they have unmet composer-based dependencies: #2494073: Prevent modules which have unmet Composer dependencies from being installed However, that issue is moving slowly, and might not be able to address dependencies of core extensions.
Therefore: We need to turn simpletest into a special case and give it the ability to check whether PHPUnit is installed.
Proposed resolution
- Add a PHPUnit check to simpletest_requirements().
- Add a PHPUnit check to run-tests.sh.
Add acomposer.json
file to the simpletest module, though currently we have no mechanism by which Composer can discover it.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff.txt | 464 bytes | Mile23 |
#11 | 2780093_11.patch | 1.56 KB | Mile23 |
#5 | interdiff.txt | 1.11 KB | Mile23 |
#5 | 2780093_5.patch | 2.01 KB | Mile23 |
#2 | 2780093_2.patch | 2.01 KB | Mile23 |
Comments
Comment #2
Mile23Patch...
Comment #3
Mile23Comment #4
dawehnerIn general this issue is a great idea IMHO
Potential Nitpick award winner: Either use ::class or at least go with either trailing slash and without training slash but be consistent :)
Any reason to not go with "Found" and "Not found"? Enabled is a bit of an odd word for a PHP library
Comment #5
Mile23Interestingly... there are no CS guidelines for .sh files. :-) Also, can't use ::class because it might not exist.
Addressed.
I really should have opened this up as an 8.2.x issue, because it improves testing, which I believe is allowed during beta. It applies either way.
Comment #6
dawehnerThank you @Mile23!
Fair point :)
Comment #7
Eric_A CreditAttribution: Eric_A commentedThis might even be considered a bug fix, given that after
composer install --no-dev
simpletest module will crash when users enable it.The module level composer.json does not seem to be needed for this task, but surely would be a nice kick-off for a feature request like #1398772: Replace .info.yml with composer.json for extensions.
Comment #9
Mile23Moving back to RTBC, re-running tests.
Comment #10
alexpottLet's not introduce a core module composer.json until the approach and what should go in them is agreed. Everything else in the patch looks fine.
Comment #11
Mile23Reasonable. :-)
Comment #12
dawehnerWell fair
Comment #13
Mile23Comment #14
alexpottTesting the premise of the issue...
However if you go to admin/config/development/testing you'll see
So there is a true dependency.
Comment #15
alexpottCommitted and pushed b8500a9 to 8.3.x and bdf69f9 to 8.2.x. Thanks!
Comment #18
Mile23rm -rf vendor/phpunit
removes the vendor directory, but doesn't change the autoload dump. Since PHPUnit generates a giant classmap and doesn't use PSR-0/4, the class 'exists.'A better demo would be
Should we follow-up on the scenario from #14? This seems like a good best-faith effort already.
Comment #19
alexpott@Mile23 yep I think you're right the fix is fine. Hence my commit.
Comment #21
Mile23Hmm... As it turns out, this was never applied to 8.1.x (or lower). So therefore we still can't package 8.1.x without dev. Unless someone were to file an issue like this: #2804663: Have simpletest, run-tests.sh enforce their dependency on PHPUnit in 8.1.x
Comment #22
Mile23