node_modules_installed() implementation not invoked (located in include file)

subscribe ;)

subscribe

Is this related to #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks ?

I haven't looked at the other issue, but yes, probably. If we move module_list(TRUE) before hook_modules_installed is executed, all modules should be listed as installed even if they are installed at once.

Note that I haven't yet tested if it works, I will soon.

Ok, attached is a first working patch.

It is quite ugly, but it seems work, comments are welcome ;) It is quite complicated, there are several things that do *not* work:

- It does not help to rebuild the module list and registry in drupal_install_modules() because the new modules haven't been enabled yet, but it needs to be done before the installed hook is invoked
- the information about the new installed modules needs to be available when calling the installed hook
- the node type static cache needs to be cleared after new modules have been installed, but it can't be done in module.inc as node_type_clear might not yet be available.

What I had to do to get it working:

- move the install stuff to module_enable
- move the invoking of hook_modules_installed to module_enable
- add a node_type_clear() to user_modules_installed().
- pass through $disable_modules_installed_hook to module_enable()

With all those changes and chx idea in #472684-25: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks, it would imho make sense to combine drupal_install_modules() and module_enable() to a single function, maybe module_install() or something like that.

Added comments to the changes in module_enable, as chx requested.

Testbot, I don't believe you :)

+    // Clear node type cache for node permissions.
+    node_type_clear();

This needs to move to node_modules_installed().

It needs to be run before user_modules_installed(). If we move it, we have to give node.module a lower weight.

n < u, right?

Sounds about right ;)

cvs up -dPC
patch -p0 < admin_role_fix_3.patch
droped all table, and used install.php
turned on the poll module (resolve the #502290: Node type related module install error. We did not empty the cache somewhere. first problem)
turned off the poll module
uninstalled the poll module
turned on the poll module
i saw following:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-create poll content' for key 1: INSERT INTO {role_permission} (rid, permission) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 3 [:db_insert_placeholder_1] => create poll content )  in user_modules_installed() (line 942 of /home/pp/munka/drupal/drupal-cvs/modules/user/user.admin.inc).

Is it a different problem?

Hm, interesting.

However, as you can see, the permission name is correct ("create poll content"), without the patch, the name of the node type would be missing ("create content"). So I assume something was messed up. Does that happen with other modules, like book, forum etc. too?

Also, were there any other error messages?

We need imho a proper permission api, to add/update/delete permissions for roles and that should try/catch that exception and display a "nice", meaningful error message, but not in this issue.

Forum and book also has this bug, but blog hasn't install/uninstall functions. (I can't uninstall blog module. :))

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-administer book outlines' for key 1: INSERT INTO {role_permission} (rid, permission) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 3 [:db_insert_placeholder_1] => administer book outlines )  in user_modules_installed() (line 942 of /home/pp/munka/drupal/drupal-cvs/modules/user/user.admin.inc).

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-administer forums' for key 1: INSERT INTO {role_permission} (rid, permission) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 3 [:db_insert_placeholder_1] => administer forums )  in user_modules_installed() (line 942 of /home/pp/munka/drupal/drupal-cvs/modules/user/user.admin.inc).

pp

> administer book outlines
> administer forums

These names are fine and are even module specific.

Hm, uninstall.... Now, I see the problem :) This is another problem. The current hook does unconditionally add all permissions, and does not check if they already exist. This is not related to the node_type/registry cache. Can you please open a new bug report about changing that query from db_insert() to db_merge()?

Then we can commit this....

I do it. #503554: user_modules_uninstalled() is missing

Committed to HEAD! Thanks.

Followup: node_modules_installed() should let us get rid of the additional node_type_clear() in DrupalWebTestCase::setUp(), that was added as a hotfix to let 'Body as field' get in.

Requires bot confirmation, of course.

Hah, doesn't work because DrupalWebTestCase::setUp() uses drupal_install_modules($disable_modules_installed_hook = TRUE), so node_modules_installed() is precisely not invoked there.
Nevermind.

When manually invoking module_enable() from within another module (see admin_menu's "Enable/Disable developer modules" link below the icon), I get the following warning now:

Warning: array_filter(): The second argument, '_drupal_install_module', should be a valid callback in module_enable() (line 200 of includes\module.inc).

Seems like a drupal_function_exists() is in order to ensure that function is available.

Please try the attached patch.

Note that this is just patchwork.. we really need to clean up that mess and probably merge drupal_install_modules() and module_enable() somehow, including dependency handling and stuff like that.

+++ modules/node/node.admin.inc	24 Jun 2009 11:01:16 -0000
@@ -634,3 +634,11 @@ function node_multiple_delete_confirm_su
+/**
+ * Implement hook_modules_installed()
+ */
+function node_modules_installed($modules) {
+  // Clear node type cache for node permissions.
+  node_type_clear();
+}

This code still lives in node.admin.inc. I do not think it's executed there.

8 days to code freeze. Better review yourself.

...which also means it can simply be removed. ;)

Not critical. Please read and understand Priority levels of Issues, thanks.

+ new title + WTF?! ;)

Sorry, I hope this revised title makes it clear... node_modules_installed() does not live in node.module, and since the registry is gone, it doesn't look like it will be found/invoked.

We probably want to just delete that code, because it's not executed currently anyway.

Doesn't look to have an overlap with the other issue.

Committed to HEAD. Thanks!

I have the same problem with Drupal 7. How can I install this patch?

I'm having the same problem. Can't install any new modules; the site is throwing this registry error, and now is throwing it when I try to clear the caches. Am I misunderstanding that this was considered fixed, or was this patch not applied?

Whoops. I think I broke it. *headdesk* Well, if anyone can help I'd be grateful.