Package Manager should keep an audit log of changes it applied to the active codebase

We can subscribe to PreRequire and keep a record of what was request to compare against what the final packages that are changed..

👍

I think it would confusing to have the PreRequireEvent logging package changes that never actually get applied to the site.

💯

I think this follow-up should the priority of "minor"

Sure. But … it will be critical for adoption/support by hosting companies, because this would make their job of supporting customers actually manageable.

This would also really help with debugging/writing the Build test over at #3355446: [PP-1] Implement experimental cron updates for contrib in automatic_updates_extensions.

This is on the right track!

High-level next steps:

1. I think the current MR is interpreting the issue summary a bit too literally, which is why it currently only works for UpdateStage instances and not all StageBase instances. Explained on the MR how that can be supported 😊
2. As for how to adopt this and start using it in tests: \ColinODell\PsrTestLogger\TestLogger to the rescue! This MR should update PackageManagerKernelTestBase to add an additional logger to the new logger.channel.package_manager_audit_log channel that this MR introduces, just like we already do at the end of \Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::setUp():
   

       $this->auditLogger = new TestLogger();
       $this->container->get('logger.channel.package_manager_audit_log')
         ->addLogger($this->auditLogger);
   

3. The preceding point should not cause any changes in existing tests. It just makes it possible for tests to add additional assertions. Let's start by adding a few. Nope, that can't work, because none of the Kernel tests execute a whole stage life cycle 🙈
4. Attempt #2! As for how to adopt this and start using it in tests: \Drupal\Tests\package_manager\Build\TemplateProjectTestBase::assertExpectedStageEventsFired() provides an example! That is using the test-only package_manager_test_event_logger (which is still valuable on its own). This MR needs to add a TemplateProjectTestBase::assertPackageManagerAuditLog() method with logic modeled after ::assertExpectedStageEventsFired().
5. 100% of build tests should be updated to use it — that means 5 classes should gain a ::assertPackageManagerAuditLog() all the way at the end.

#16 helped me discover the need for #3359609: Document that validators comparing state between stage life cycle operations must store that state out-of-band, not on the object 👍😄

Looking great! 👏 😄

I requested some changes (pun intended 😜) to help improve test code clarity. But this is definitely close to being done!

Credited @omkar.podey for the review.

I found more room for simplification!

99% ready!

No more need for a follow-up for a Drush command: I discovered drush watchdog:list --type thanks to @tedbow's comment #8.

Great to have this done!

Nice catches from @phenaproxima. Found a few new inconsistencies now — see the MR.

Once tests are passing again (after #3362110: Since #3361983 was committed to Drupal core, psr/http-message needed to be explicitly required for build tests lands), this can be committed 👍

🏓 See MR comment: https://git.drupalcode.org/project/automatic_updates/-/merge_requests/86...

think we would need to expand this issue a lot to make including the messages from updates that did not succeed or were canceled to be understandable by people were not involved in developing this module.

Fair!

That will fulfill the primary purpose of the of the issue which is let the admin know what projects have been changed by Package Manager.

WFM

Found some problems during re-reviewing.

For @phenaproxima's feedback.

One last piece of feedback.

P.S.: Those new commit messages are WAY better! 👏

🚀

The reason I was wondering is because right now we don't test that nothing gets logged if the process is stopped in preapply or before

If this were critical logic, I'd get it. But it's just logging.

maybe we don't need to test this but […]

I don't think we need to test this.

The complexity of testing would far outweigh the complexity of the logging. Plus, a bug in the logging cannot even break the functionality itself. Hence no tests for edge cases necessary, only for The Happy Path.

To clarify #55: @wendyZ specifically requested this behavior; they were surprised it did not already happen, thus confirming the need for this functionality — here's what they wrote:

But the whole update process was not logged at all. By looking at the log report, it only shows the 2 auto_updates modules are installed. It should also log the core was upgraded from which version to version.

    // The first record should be of the requested changes.
    $expected_message = <<<END
Requested changes:
- Update drupal/core-recommended from 9.8.0 to ^9.8.1
- Install drupal/semver_test * (any version)
END;
    $this->assertSame($expected_message, (string) $records[0]['message']);

    // The second record should be of the actual changes.
    $expected_message = <<<END
Applied changes:
- Updated drupal/core from 9.8.0 to 9.8.1
- Updated drupal/core-dev from 9.8.0 to 9.8.1
- Updated drupal/core-recommended from 9.8.0 to 9.8.1
- Installed drupal/semver_test 8.1.1
- Uninstalled package/removed
END;
    $this->assertSame($expected_message, (string) $records[1]['message']);

🤩