Improve hook_install() to include recommendations for config import and pass an $is_syncing argument

Bump!!! =]

I had the following problem:

The built-in config installer and (also the config_installer contrib) are giving me some hard times today...
* There are module A and module B.
* Module B depends on module A.
* Module A provides an entity type with entity view and form display modes and some fields. XY field on the entity type is not displayed by * default.
* Module B would like to make XY field visible on the entity type's form display mode when it gets installed.
We implemented this in a hook_install() and it worked perfectly, but with config installer(s) it does not work. The form display mode config does not exist when Module B's install hook is running, although Module A is already enabled. Tried to clear various caches, without any luck... Also checked some config import-related events, none of them seemed a reasonable replacement for this task.
Who has any idea what is going on here? (Drupal 8.7.10)

@berdir Highlighted on Slack that I should run if (!\Drupal::service('config.installer')->isSyncing()) {} in install hooks every time before I try to modify a config. That fixed the problem.

@alexpott mentioned in slack that it might be worthwhile to pass an is_syncing argument to the hook so it was more obvious that you needed to make a decision based on that.

I agree with that decision, updating the IS.

something like so

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -220,15 +221,32 @@ function hook_modules_installed($modules) {
-function hook_install() {
+function hook_install($is_syncing) {

Could the param for this be $is_syncing = FALSE just to avoid changes being made to all existing hook_installs? Is that possible in this case?

Could the param for this be $is_syncing = FALSE just to avoid changes being made to all existing hook_installs? Is that possible in this case?

This is just a sample implementation, and believe it or not, hooks don't have to add the argument if they don't want too (i.e. it will keep working without everyone having to change their implementations).

This is not dissimilar to hook_update_N if you want to use the $sandbox variable, you declare it as an argument, but if you don't it still keeps working. So I think we're good here - as you can see from the patch - I didn't change any of the other implementations, only forum module, but tests still passed.

Sure

That all looks good to me. Patch applies, standard profile and forum module all correctly enable. The install tasks for the forum module create the term as expected. All looks good to me.

I think we should change core/profiles/demo_umami/modules/demo_umami_content/demo_umami_content.install to use the new system as well, since that is using the service call. I couldn't find any more examples besides that in core.

Happy to RTBC once that is done or if you have a reason why you don't want to do that in this patch. :)

Oops it's not demo_umami_install()...

Yep, don't rush stuff

Fixed and added the equivalent for uninstall, and modules_installed, modules_uninstalled

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -354,6 +354,8 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+    $config_installer = \Drupal::service('config.installer');
+    $sync_status = $config_installer->isSyncing();

Nitpick - Can this be one-lined? Also happy to be overridden on this.

Sure

Added https://www.drupal.org/node/3098920 for change record

So sorry - Just did a last review and there is still one more - core/modules/media_library/media_library.install with !\Drupal::isConfigSyncing() in it.

nice!

Patch still applied, All config sync checks in install/uninstall hooks are changed.

Change record makes sense to me. Happy to mark as RTBC - Assuming the tests all come back green. :)