Updated registry not available in hook_modules_installed()
catch - June 4, 2009 - 19:51
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | duplicate |
| Issue tags: | Registry |
Description
Splitting this from #480660: Add an 'administrator' role to core since it's a registry issue and needs a proper look. Attaching Berdir's test changes which demonstrate the (currently untested) notices in HEAD.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| admin_role_fix.patch | 777 bytes | Idle | Failed: 11835 passes, 0 fails, 11 exceptions | View details | Re-test |
#1
subscribe ;)
#2
subscribe
#3
The last submitted patch failed testing.
#4
Is this related to #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks ?
#5
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.
#6
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.
#7
Added comments to the changes in module_enable, as chx requested.
#8
tagging.
#10
The last submitted patch failed testing.
#11
Testbot, I don't believe you :)
#12
<?php+ // Clear node type cache for node permissions.
+ node_type_clear();
?>
This needs to move to node_modules_installed().
#13
It needs to be run before user_modules_installed(). If we move it, we have to give node.module a lower weight.
#14
n < u, right?
#15
Sounds about right ;)
#16
Marked #502290: Node type related module install error. We did not empty the cache somewhere. as duplicate.
Patch is looking much cleaner, let's get it in.
#17
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?
#18
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.
#19
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
#20
> 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....
#21
I do it. #503554: user_modules_uninstalled() is missing
#22
The last submitted patch failed testing.
#23
o rly.
#24
The last submitted patch failed testing.
#25
#26
Committed to HEAD! Thanks.
#27
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.
#28
The last submitted patch failed testing.
#29
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.
#30
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.
#31
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.
#32
The last submitted patch failed testing.
#33
Duplicate of registry rollback.