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
8.x-1.x currently does not merge require-dev values from modules' Composer files. This is slightly problematic when setting up integration tests for a module. The test build can of course add dependencies itself, but it's less maintainable and completely bypasses Composer.
Proposed resolution
Merge require-dev values just like require values are merged already.
Remaining tasks
N/A.
User interface changes
None.
API changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | composer_manager_2490480_12.patch | 4.76 KB | caseyfw |
| #10 | composer_manager_2490480_10.patch | 4.76 KB | caseyfw |
| #5 | composer_manager_2490480_5.patch | 4.06 KB | Xano |
Comments
Comment #1
XanoI'll post a patch.
At first glance this seems to be a regression when compared to 7.x-2.x. Is there a specific reason this merge was left out?
Comment #2
XanoI performed a manual test locally and this works. I did have to remove the check for
requirevalues, as my module does not have any regular Composer dependencies, but onlyrequire-devones.It does look like Composer Manager cannot handle conflicting dependencies: it simply merges all dependencies based on their name and not the version constraints, which dependencies cannot be fully checked when running
composer installand code has the potential to fail during runtime.Comment #3
XanoFixed the tests too.
Could someone enable automated testing for this project so the patch can be reviewed?
Comment #4
bojanz CreditAttribution: bojanz at Centarro commentedWe should check if the composer file has a require or require-dev dependency, instead of removing the require check.
Most modules will ship with composer.json files, but many won't have any composer dependencies, so they don't need to be merged in.
The rest looks fine.
Correct. We have access to Composer classes so we could resolve dependencies, but it's probably not worth it to spend the time since we're working on actively making composer_manager unnecessary.
Comment #5
XanoComment #6
bojanz CreditAttribution: bojanz at Centarro commentedCommitted, thanks.
Comment #9
bojanz CreditAttribution: bojanz at Centarro commentedI had to revert this :(
It breaks Composer when require-dev is empty, cause the braces are not curly enough:
" [Composer\Json\JsonValidationException]
"./composer.json" does not match the expected JSON schema:
- require-dev : array value found, but an object is required "
https://travis-ci.org/commerceguys/commerce/jobs/63786299
Comment #10
caseyfw CreditAttribution: caseyfw commentedThis is just an issue because json_encode has no idea if an empty config element should be an array or an object. A solution would be to strip empty config elements out before writing the JSON.
Comment #11
caseyfw CreditAttribution: caseyfw commentedComment #12
caseyfw CreditAttribution: caseyfw commentedMissed an erroneous bit of boolean logic in RootPackageBuilder.
Comment #13
joshtaylor CreditAttribution: joshtaylor commentedThis patch makes composer_manager work again with core.
Comment #14
CalebD CreditAttribution: CalebD commentedCan we get composer_manager_2490480_12.patch committed and a new release for 8.x published?
I just spent the last couple hours confusedly trying to get composer and Composer Manager to work, not realizing that the current release has the older patch applied, the 8.x-1.x branch has the older patch reverted, and this patch is against 8.x-1.x and doesn't apply to the current 8.x-1.0-beta2 release.
After sorting through all this, I was able to get things working. As it stands right now, 8.x-1.0-beta2 is broken and 8.x-1.x with this patch works well.
Comment #16
bojanz CreditAttribution: bojanz at Centarro commentedRecommitted, thanks everyone.