Do not assume that Composer package names will exactly match Drupal projects names

The changes are straightforward enough, but they are not taking into account the possibility that ComposerUtility's mapping methods might return NULL if they can't resolve the package or project name.

I'm not sure what the correct error handling is -- silently skip, or throw an exception? Assigning to @tedbow to get his opinion.

Sorry to just "throw this over the wall", but I just happened to run across something relevant and wanted to make sure it's surfaced and I don't have time to read all the comments to see if it already has been...

Don't forget the case of the Drupal.org Composer Facade's submodule disambiguation feature, which can lead to module names like this when there's a case of a project's submodule using the same machine name as a top-level project:

drupal/acquia_telemetry-acquia_telemetry

in that case both package names do refer to the same project name. is that right?

I don't think so, if I understand your question, @tedbow. The whole thing's a bit of a mess, frankly. Here's what I know:

1. drupal/acquia_telemetry and drupal/acquia_telemetry-acquia_telemetry refer to two completely different projects with different codebases:

   $ composer info -a drupal/acquia_telemetry | grep 'source :'
   source : https://git.drupalcode.org/project/lightning_core
   
   $ composer info -a drupal/acquia_telemetry-acquia_telemetry | grep 'source :'
   source : https://git.drupalcode.org/project/acquia_telemetry
     

2. But they have the same project name in their composer.jsons (drupal/acquia_telemetry), and their .info.ymls are both named acquia_telemetry.info.yml:
3. But composer require them both in your Drupal project, and they'll both get installed, just in different directories:
   • modules/contrib/acquia_telemetry
   • modules/contrib/acquia_telemetry-acquia_telemetry

Well, @tedbow, in a quick test in my local dev environment it won't let me composer require them together anyway. So I can't readily reproduce it, and I forget the details from two weeks ago. Take it for what it's worth and do what you will with it, I guess. ¯\_(ツ)_/¯

Review posted on MR.

Unfortunately, Wim it seems you reviewed the wrong MR

9 remarks posted on the MR. But … this really needs input from @tedbow, I can't properly review this quite yet.

#3318625: Remove active composer fixture files in UpdaterFormTest landed, so unpostponing

Given #3322917: Create a test (or tests) to prove Package Manager works with submodules as implemented by packages.drupal.org, I think the ability to correctly handle submodules is critical missing functionality.

Handling that is out of scope here. Let's use #3322917: Create a test (or tests) to prove Package Manager works with submodules as implemented by packages.drupal.org for that. Comments #10 through #14 are relevant for this, will copy them to #3322917.

Making issue metadata complete: @omkar.podey created #3320661: Update $supported_package_types in automatic_updates_extensions/src/Form/UpdateReady.php in response to @phenaproxima and @tedbow at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/48...

Since this is in the final stages, I'm looking at this with a very strict eye, to ensure that it is a net improvement to test clarity (because the pre-existing tests definitely lack clarity).

So … 6 remarks on the MR for that — but once they're addressed, this will be a net improvement! 👍

3 last remarks: 1 applause, 1 test approach change request, 1 docs/clarity remark. Once addressed, I will be RTBC!

Looks excellent now! 🤩

Already assigned issue credit to everyone, because there were truly contributions from all people active on this issue 😊 Team effort!

👍 Makes sense!

I am not removing the fixture because package_name_project has structure and project folder but no info about the project in installed.php and installed.json, to replicate it means to perform 2 operations , first to create the complete fixture and then remove package is what I beleive, thoughts?

This is still relevant. And given #3355463: Looking for Alpha tester for future functionality, we should probably get this done soonish.

The successor to getProjectForPackage() is \Drupal\package_manager\InstalledPackage::getProjectName().

This needs to be rebased on top of 3.0.x.

i'm wondering if the project name changes are even required now ?

AFAICT the test coverage in the old MR has not yet been ported to the new MR? 😅

Also, in reviewing the kinda tricky changes to getProjectTitle(), I discovered that it's essentially obsolete now and can be replaced with a trivial single line.

The tests in the Old MR are obsolete now so the main problem described in the issue was the assumptions about how the project name and package name were related and with the new methods (\Drupal\package_manager\InstalledPackagesList::getPackageByDrupalProjectName and \Drupal\package_manager\InstalledPackage::getProjectName) it's no more the case and it has it's own coverage in \Drupal\Tests\package_manager\Kernel\InstalledPackagesListTest

Updated the issue summary to explain testing, and quoting comment #54

The tests in the Old MR are obsolete now so the main problem described in the issue was the assumptions about how the project name and package name were related and with the new methods (\Drupal\package_manager\InstalledPackagesList::getPackageByDrupalProjectName and \Drupal\package_manager\InstalledPackage::getProjectName) it's no more the case and it has it's own coverage in \Drupal\Tests\package_manager\Kernel\InstalledPackagesListTest and the project titles has tests too \Drupal\Tests\automatic_updates_extensions\Functional\DisplayUpdatesTest::testDisplayUpdates

#3334994: Add new InstalledPackagesList which does not rely on Composer API to get package info landed February 21, 2023.

This issue was opened in August 2022, and the old MR was worked on in Sep-Nov 2022. So yeah, things have obviously changed 😅+1 for #58.

But the MR is still wrong it seems? private function getProjectTitle(string $package_name): string { is first removed and then restored. I offered a much simpler alternative at https://git.drupalcode.org/project/automatic_updates/-/merge_requests/91... that still has not been addressed?

I did leave a MR comment https://git.drupalcode.org/project/automatic_updates/-/merge_requests/91... on that. Can you take a look at it and reply if i'm missing something ?

Wow, great catch, and clear explanation! 👍

This is then essentially ready … but since we're modifying this code anyway, we should be moving it to the appropriate location now.