Improve hook_module_preinstall & hook_module_preuninstall to include recommendations for config import and pass an $is_syncing argument

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Probably the replacement for workspaces hook 🐛 Add/remove event listeners provided by a module on module install/uninstall Needs work

Drafted CR https://www.drupal.org/node/3346540 →

And fixed the demo_umami_content_module_preinstall()

No tests added for #2914174: Improve hook_install() to include recommendations for config import and pass an $is_syncing argument → so I think it's ok here too

Surprised the previous one didn't need tests but if they're not needed!

Changes in #22 look good and the CR makes sense to me.

Will mark it but wonder if it should default to FALSE?

There's no default, it's part of state of (de)install process

The last submitted patch, 22: 3118155-22.patch, failed testing. View results →

Known random fail in Ckeditor5.Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest

Review in #8 hasn't been addressed.

This came up as the daily BSI target

This actually to have resolved in 🐛 ConfigInstaller::isSyncing() is not reset before module preinstall Fixed

The issue is still valid - this hooks

1. +++ b/core/modules/workspaces/workspaces.install
   @@ -30,8 +30,8 @@ function workspaces_requirements($phase) {
   -function workspaces_module_preinstall($module) {
   -  if ($module !== 'workspaces') {
   +function workspaces_module_preinstall($module, bool $is_syncing) {
   +  if ($module !== 'workspaces' || $is_syncing) {
   

   not sure it needed today

2. +++ b/core/profiles/demo_umami/modules/demo_umami_content/demo_umami_content.install
   @@ -10,8 +10,8 @@
   -function demo_umami_content_module_preinstall($module) {
   -  if ($module === 'demo_umami_content' && !\Drupal::service('config.installer')->isSyncing()) {
   +function demo_umami_content_module_preinstall($module, bool $is_syncing) {
   +  if ($module === 'demo_umami_content' && !$is_syncing) {
   

   this change still missing

So it's no longer a bug

Mind updating the summary then please.

updated IS, as I greped for hook the typo fix added

Cool thanks.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11654

The change record for this issue has not been reviewed.

There shouldn't be a change record for this issue because the API already exists. Not sure what to do with a draft change record that should be abandonedz can we just delete it?

Yes, just delete it. Otherwise it merely clutters the list of change records and makes them harder to manage.

I have deleted it. Thanks catch.