Module weight is not taken into account after module installation

IMHO this is not the bug. You should be able to have a fully working system when hook_install() is called, which requires the kernel updating to be done.

The bug is that changing the configuration, so the module weight, doesn't refire a container rebuild.

There is a very simple test for this:

* create 2 modules: module_parent and module_child
* add *.install to both modules implementing hook_install() (update according to the modules name):

function module_parent_install() {
  module_set_weight('module_parent', 1);
}

* make module_child dependent of module_parent
* install module_child

Result:
* module weight of module_parent is "0"
* module weight of module_child is "1"

For me, simply reloading the extension config did the trick (see patch).

Let's adapt the title to be more sane.

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -136,6 +136,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        // Reload config to detect recent changes.

It would be great to explain why it changes, so for example point to module_set_weight()

Updated the comment and added a test for this.

I took me a while to unterstand the root cause of this issue. I tried to improve the comment a bit.

Out of my own interest I ran the test locally without the fix, so yeah this fix indeed fixes the problem. From my point of view the fix seems totally logical.

I've tested the patch manually to see it fixes Tour builder #2884881: Operations are not updated on the demo site (Cache clear vs drush cache-rebuild). And it does not fix it.

Comparing the UI Performance / Clear cache and drush cache-rebuild vs this patch

I wonder why
- there is no UI for rebuild all?
- drush using drupal_rebuild

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -136,6 +136,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+        $extension_config = \Drupal::configFactory()->getEditable('core.extension');

Is this enough comparing with drush cache-rebuild

My tour_builder hook_entity_type_alter is not called after installing the module. function tour_builder_entity_type_alter(array &$entity_types) { overrides the TourUiListbuilder and adds Operations.

Steps to reproduce

1. drush @drupal.d8 pm-uninstall tour_builder --yes
2. Visit http://drupal.d8/admin/config/user-interface/tour and check the Operations (OK: Edit / Delete)
3. drush @drupal.d8 pm-enable tour_builder --yes
4. Visit http://drupal.d8/admin/config/user-interface/tour and check the Operations (NOT OK: Clone / Export* missing)
5. drush @drupal.d8 cache-rebuild
6. Visit http://drupal.d8/admin/config/user-interface/tour and check the Operations (OK: Edit / Delete / Clone / Export*)

applying the patch I can confirm the module weight is adjusted using Devel PHP

Checking Drush calling drupal_rebuild() and adding drupal_flush_all_caches(); fixes my hook weight getting respected.

1. drush @drupal.d8 pm-uninstall tour_builder --yes
2. Visit http://drupal.d8/admin/config/user-interface/tour and check the Operations (OK: Edit / Delete)
3. drush @drupal.d8 pm-enable tour_builder --yes
4. Visit http://drupal.d8/admin/config/user-interface/tour and check the Operations (OK: Edit / Delete / Clone / Export*)

@clemens.tolboom That's unfortunate that this patch does not fully solve your issue. But since this is a pretty intricate bug, it seems like we could solve the additional cache clear with dedicated tests in a follow-up? I don't think we can get away with the drupal_flush_all_caches() there, because that will make the Drupal installation much slower, which would be very unfortunate. So maybe there is another more fundamental cache invalidation bug that you have uncovered and the cache clear just fixes the symptom by wiping all caches. In any case, do you agree to move this to a follow-up? Would be awesome to get this nasty bug fixed...

I've created #2886083: After installing tour_builder at least hook_entity_type_alter is not called by modules weight.

To not derail this issue any further I re-added patch #10

Thanks a lot, that's nice. See you over there, then ;-)

Indeed RTBC

Build failure is out of disk space

11:31:14 PHP Warning: file_put_contents(): Only 0 of 2413 bytes written, possibly out of free disk space in /opt/drupalci/testrunner/src/DrupalCI/Build/Build.php on line 468

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -136,6 +136,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        $extension_config = \Drupal::configFactory()->getEditable('core.extension');
   

   this is yuck, because the dependency on the config factory is hidden, but its pre-existing so untangling that is out of scope.

   Twice in this method we're calling \Drupal::configFactory() - can we store a reference to that in a local variable instead to remove the duplicate call to the container? When we eventually get to make the dependency explicit, having the single variable will help with refactoring too.

2. +++ b/core/modules/system/tests/modules/module_handler_test_multiple/module_handler_test_multiple_child/module_handler_test_multiple_child.install
   @@ -0,0 +1,15 @@
   + * Install, update and uninstall functions for the
   + * module_handler_test_multiple_child module.
   

   does this needs to be one line?

3. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
   @@ -44,4 +44,21 @@ public function testRouteRebuild() {
   +   * Tests that configuration changes done in hook_install() are saved for
   +   * dependent modules.
   

   does this has to be less than 80 chars according to our phpcs?

Re #21
1. It looks to me like the second call needs to be there, so I have left this as it is
2. & 3. - I have updated the comments here.

Note: storing a local variable is tricky. It might be the case that the kernel was rebuild in the meantime. This is why #2380293: Properly inject services into ModuleInstaller is not as easy as it sounds like in the first place.

The patch in #22 introduced some whitespace in module_handler_test_multiple_child.install which caused the test to fail.

Thanks! Back to RTBC, then.

Agreed with getting this in as-is with the test coverage, the issue to do DI properly is already open, but having the system functionally correct only helps with that.

Just for the record, this broke the Lingotek module tests, which depends on content_translation, which has a module_set_weight in hook_install.

Not sure if this needs a change record and possibly could break other modules.

lingotek #2892220: Change weight of the lingotek module after content_translation changed

@penyaskito regarding a CR this was a bug fix so I guess it needs no CR.

In theory, yes, it's a bugfix so no CR needed. In practice, modules may break or have problems. E.g., if content_translation was not enabled when lingotek is installed, content_translation weight will be 0 instead of the 10 that would be expected (see content_translation_install().

There are 48 implementations of hook_install in core, the ones using module_set_weight which may need a hook_update for updating the weight:

• content_translation
• forum
• menu_link_content
• page_cache_form
• views
• and some test modules that I don't think we care for this matter.

Also very popular modules like paragraphs have a similar issue:

function paragraphs_install() {
  // Assign a weight 1 higher than content_translation to ensure paragraphs_module_implements_alter
  // runs after content_translation_module_implements_alter.
  module_set_weight('paragraphs', 11);
}

Also pathauto changes its weight in hook_install.