Package Manager should have status report entry to inform user of Composer version being used

Wim Leers → credited tedbow → .

Discussed with @tedbow, pulling into sprint.

The last submitted patch, 5: 3354914-5.patch, failed testing. View results →

+++ b/package_manager/package_manager.install
@@ -23,6 +25,14 @@ function package_manager_requirements(string $phase) {
+    'title' => t('Composer version'),

Maybe
'Package Manager Composer version'

To give the user more context

or we could change the title to "Package Manager"
and add more details

Composer version: X
Composer path: Y
File copier in use: Rsync

I first thought the same, but … is helpful to know in general, I think?

Also, the best practice is AFAICT to have separate entries for each semantically separate thing? For example, jsonapi_requirements() generates separate entries for:

• JSON:API multilingual support
• JSON:API revision support
• JSON:API allowed operations

I first thought the same, but … Composer version is helpful to know in general, I think?

Yes but I think we want to be very clear incase there multiple versions of composer on the system. The admin should not assume this what would used on the command line by default and should not assume whats on the command line will be used by Package Manager

Now it looks like

Could you add tests or update current ones and reassign ?

Do we really need a test for this as there is nothing to test here, it just shows some information on status report page?

Yes we need to test a few things

1. We need to test if we override the config package_manager.settings.executables.composer to different path that this shows up in the status report and that if we don't it points to the actual composer
2. that all the labels show up
3. if we change the syncer to php then it also changes in the report
4. We can't really test changing the version but just test that current version is shown

Relating to 📌 Warn about package_manager errors and warning on the status report page Needs work because this would also show an error if we can't find the composer executable.

I think this is a great start! But I think we could sort of consolidate the requirements to make them a little more useful and clean.

Discussed with @Wim Leers and we understand why a simpler test is preferable but that mocking of composer is not possible in a functional test. But converting this to a kernel or unit test is not possible because we need to test hook_requirements()?
@phenaproxima could you please clarify as you suggested in scrum that maybe we could try to do mocking of composer to simplify this test?

What do you think of this?

This is a simpler test because we're not testing as much stuff. I'm not sure we get a ton of benefit by testing different combinations of file syncer + executables, for the simple reason that they are separate requirements that don't affect each other. So we don't really need to test them in combination, do we?

Yes, I do agree that we don't necessarily have to test different combinations of file syncer + executables, and now the test is much simpler👍🏻

Thanks, @kunal.sachdev! Assigning to @tedbow for final review, since he's really good at finding important things others might have missed...

I see I was the one that suggested putting rysnc info on this in #8 but that was before 📌 Warn strongly if the rsync file syncer is not in use Fixed was committed. Now everybody should using rsync or they will already get a warning that they are using something that is not recommended.

In light of that I think we should remove the rsync section. Also see my MR comments

Attaching screenshots of how the requirement looks now.

I've addressed all of @tedbow's feedback that I can. Reassigning for re-review.

@phenaproxima thanks for addressing my feedback.

See MR comments

Looks good! Thanks

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