Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
While working on ComposerPluginsValidatorTest, I over time (in #3331168: Limit trusted Composer plugins to a known list, allow user to add more and #3344127: Run `composer validate` after FixtureManipulator commits its changes) accumulated two @todos:
-
// @todo Uncomment this in https://www.drupal.org/project/automatic_updates/issues/3252299 -
// @todo handle following type of case where the project is invalid in // https://www.drupal.org/node/3344595
… but #3252299: Reliably support cweagans/composer-patches in Package Manager & Automatic Updates: validate stage and #3344595: ComposerInspector::validate() should run `composer validate` have both landed since then. 😅
Proposed resolution
Do whatever work is necessary to get rid of these @todos.
Remaining tasks
TBD
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork automatic_updates-3348159
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
yash.rode commentedComment #5
yash.rode commentedNeed help wtih
\Drupal\Tests\package_manager\Kernel\ComposerPluginsValidatorTest::testValidationDuringPreApplywith the dataproviderproviderSimpleValidCases(). What should be the the value of extra for fakecweagans/composer-patches.Comment #7
yash.rode commentedShould we create two separate data providers for
\Drupal\Tests\package_manager\Kernel\ComposerPluginsValidatorTest::testValidationDuringPreCreateandtestValidationDuringPreApplyasproviderSimpleValidCasesshouldn't haveIt cannot be installed by Package Manager.for the earlier one and should be there for the later one.Comment #8
tedbow@yash.rode I think we can just add this message in `testValidationDuringPreApply` directly to the validation result.
I have pushed up a fix
Comment #9
yash.rode commentedComment #10
yash.rode commentedRemoved other todo.
Comment #11
yash.rode commentedComment #12
wim leersPlease just mark going forward — I will have reduced capacity for reviews. @tedbow or @phenaproxima can also review! 😊
Comment #13
wim leersComment #14
yash.rode commentedComment #15
phenaproximaComment #16
yash.rode commentedComment #17
yash.rode commentedI have 2 questions for the changes requested.
Comment #18
yash.rode commentedI have a doubt, what do we want to test in 'one supported composer plugin' dataset?
Comment #19
yash.rode commentedComment #20
yash.rode commentedComment #21
wim leersAgreed with @yash.rode and @phenaproxima in #18.
But that decision seems to have left some stale/obsolete things in the MR 🤓
Comment #22
yash.rode commentedRemoved obsolete lines of code.
Comment #23
wim leersComment #24
phenaproximaI think this looks really, really good -- such a nice, clear test! I think we could streamline a thing, comment another thing...and just explain something else in an MR comment thread.
But otherwise, I really like it.
Comment #25
yash.rode commentedComment #26
wim leersComment #27
phenaproximaLooks fantastic. Made a few very minor changes for strictness' sake, but I think this is excellent; it's clean, clear, and tightly scoped. A top-nottch patch. I will commit when tests pass.
Comment #29
phenaproxima