Update functions should not invoke hooks however,

$permissions = user_permission_get_modules();

http://api.drupal.org/api/drupal/modules--user--user.module/function/use...

Quoting David Rothstein from #717834-15: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct

Reading through all the update functions, I think the biggest problem we're going to have in straightening this all out is with user_update_7006(). That function calls user_permission_get_modules() which in turn invokes hook_permission() in all enabled modules in order to try to get module-permission relationships that it can store in the database. But several hook_permission() implementations in core have dynamic permissions (node, taxonomy, etc) and therefore rely on API functions that aren't likely to work well during the update without a complicated set of dependencies in place. So basically, there's a good chance that user_update_7006() is at the heart of all our problems.

I've ignored all that for now, because user_update_7006() seems flawed anyway. People are supposed to disable contrib modules during the D6->D7 update so most hook_permission() implementations aren't ever going to be picked up by the above procedure. I think we might want to replace that update code with something totally different (e.g. code that runs on cache flushes and looks for new modules then, for example).

There's no issue open for this (the update dependencies patch is skipping trying to deal with it), so I'm opening one. Critical for two reasons:

1. Any module with dynamic permissions that is enabled while this function runs needs to be updated to the point where calling their hook_permission() implementation actually works without throwing fatal errors - this has been part of the cause of our upgrade path woes.

2. Dynamic permissions may depend on other modules that are disabled (for example modules that provide node types), so they won't get picked up by this.

3. Contrib modules are supposed to be disabled when this runs, so that won't get fixed either.

There is no way to figure this out without invoking the hooks, so I agree with David's suggestion that we look at doing this differently - for example on cron, module enable/disable or similar.

CommentFileSizeAuthor
#15 1234834_defined.patch1.54 KBcatch
#14 1234834_defined.patch1.41 KBcatch
#12 1234834.patch1.38 KBcatch
#7 1234834.patch1.38 KBcatch
#6 1234834_lets_see.patch572 bytescatch
#5 1234834_lets_see.patch749 bytescatch

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Component: base system » user.module
Issue tags: +Upgrade path

http://api.drupal.org/api/drupal/modules--user--user.install/function/us...

So let's just add that field in there and move the functional logic into user_flush_caches()?

---
Actually, why does this update and schema column exist at all? Didn't we discuss that user permissions can be shared across modules and they are not bound to modules in the first place?

catch’s picture

catch
he/him
Primary language English
commented

That's discussed in #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced.

I hate to say it but I helped with the original patch :( #306027: user_modules_uninistalled() is missing and #503554: user_modules_uninstalled() is missing have background. The patch from #503554: user_modules_uninstalled() is missing would not have been any good either, because when modules are uninstalled we can't safely run hook_permission(), and hook_permission() might itself depend on things added dynamically depending on other modules even if we could.

David_Rothstein’s picture

David_Rothstein commented

Subscribing.

For point #1 (potential fatal errors during updates as a result of this), I posted some notes here: #717834-32: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct

But for the purposes of this issue, we don't have to worry too much about that (since we're going to just try to remove it instead).

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

So here's a patch that would remove the fatal errors - by simply not attempting to match modules to permission during the update. This means a tonne of permission in the permissions table with empty strings for the module column, but with a D6 site with a lot of contrib modules, you'll also have a tonne of permissions in the permissions table with empty strings for the module column after upgrade with 7.7 anyway, so I can't get too sad about that.

This leaves a couple of options:

- since we can never run user_permissions_get_modules() during an update function, we could add a runtime check (cron, drupal_flush_all_caches() etc.) with a very long comment explaining why we're matching permissions to modules on runtime.

- we could also require all modules to update their own permission individually in their own updates (which is essentially what we do now, except we're pretending we don't).

For the former it would more or less be moving the hunk here to drupal_flush_all_caches().

For the latter, core would need to add individual updates for each hook_permission(), we could provide a helper to do the update.

Neither of these are pretty, so we could also just commit this, un-fatal the upgrade path for a large number of sites, then figure out a way to update the table properly in a new issue.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 1234834_lets_see.patch749 bytes
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 1234834_lets_see.patch572 bytes

Removing the comment, if we go with this patch we'll need a new (probably major) issue to track that and git blame will point back to this one.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 1234834.patch1.38 KB

Discussed chx in irc, he's fine with the user_flush_caches() approach and would rather just add it here (it is ugly but there are no other options for runtime maintenance of things like this).

So here's patch doing that. I didn't run tests on this one.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

I think that (bool) is not necssary but it's a pattern so let's remove 'em together in one patch. WHERE module = :empty', 0, 1, array(':empty' => '') this OTOH is very nice and cross db friendly.

As for remvoing hook invocation from updates , nobrainer.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Also agreed with the user_flush_caches() approach.

I wonder whether we need to protect this from running in MAINTENANCE_MODE though.

chx’s picture

chx commented

#7: 1234834.patch queued for re-testing.

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs work

Ok this needs work. 0 passes is not good.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1234834.patch1.38 KB

{role_permision}

Status: Needs review » Needs work

The last submitted patch, 1234834.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 1234834_defined.patch1.41 KB

It is interesting that hook_flush_caches() is running before user module is updated, that could get messy.

Let's see if we can work around it by checking for maintenance mode.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 1234834_defined.patch1.54 KB

Added a comment explaining why the maintenance mode check is there. Good prediction from sun in #9 :)

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

That's more like it.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

The patch in #15 fixes also #1245980: Outdated static schema cache while upgrading from a plain D6 install to D7 on Windows, however I don't think they are exactly the same issue.

damien tournoud’s picture

damien tournoud commented
Status: Reviewed & tested by the community » Needs work 
+/**
+ * Implements hook_flush_caches().
+ */

Ew. So, hook_flush_caches() is an *info hook*. It should *not* be used to trigger side-effects. Moreover, we do already way to much during each cron run (including DELETE and re-INSERTing all the date formats, for some unknown reason). Let's not increase the bleeding.

Also, the "very long comment explaining why we're matching permissions to modules on runtime" mentionned by @catch in #4 is not in the patch.

What is the worse that can happen if some permissions are not properly matched? Worse case scenario, we could add a (advanced) hook_module_update_starting() that would be executed in update.php just before we launch the update batch?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

I agree with @Damien Tournoud's thinking in general, but not with blocking this patch.

We've identified in several other issues already that hook_flush_caches() is over-used currently, to not only flush essential core caches, registries, less important module caches, but also to rebuild system data. Way too much is happening there, with endless interdependencies, and we've lost control over it. Prime example and stop-gap fix attempt being #996236: drupal_flush_all_caches() does not clear entity info cache

However, that's an issue we have to fix either way for D8 - most likely, by introducing a separate hook_rebuild(). One more or one less abuse of hook_flush_caches() doesn't change the situation.

Note that this mapping of permissions to modules only happened to be triggered in a module update, but thus far, is unrelated to the database update system. The module information is primary used by user_modules_uninstalled() (possibly elsewhere too, not sure).

catch’s picture

catch
he/him
Primary language English
commented

See I knew someone would go eww which is why I originally posted #6. If one of you wants to rtbc that then great, otherwise good luck on this issue because I'm done with it at this point.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

http://drupal.org/node/1234834#comment-4836176 then let's get this one in ASAP. It's a critical. The other one won't be. Opened #1248742: User permissions are not associated with modules

Starminder’s picture

Starminder commented

+1

jantoine’s picture

jantoine commented

+1

*edit*
Additional review confirming that the patch solves the reported issue, please commit.

chx’s picture

chx commented

+1 means what? Do you have some issues that got solved? Or are these a spammer's attempt to verify we dont block accounts when they post junk?

Starminder’s picture

Starminder commented

@chx: +1 means I want to be notified when this gets updated, as this issue is near and dear to my heart. It's a few characters shorter than typing out subscribe, as sometimes I need to jump on multiple threads as reminders of where my problems are. I'm hoping this one solves some major upgrade issues I have, looking forward to a fix being committed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Talked through this with catch on IRC a bit.

I'm not a huge fan of the solution in #6. I tested this on a working D7 database by setting the 'module' column from 'dashboard' to '', and then uninstalling Dashboard module, re-saving the permissions page, clearing the cache, etc. leaves that cruft in the database forever. This basically makes fixing something like #607238: Permissions are assumed to be unique among modules, but uniqueness is not enforced impossible until we find a workaround. #1248742-1: User permissions are not associated with modules has an interesting idea of how to handle that.

However, since this situation already exists with all contributed and installed-but-not-enabled-at-the-time-of-update core modules, and the current state of the code means fatal errors, the solution in #6 is better than the current situation. (I know I'm not supposed to edit old update functions, but in this case I don't think we have a choice.)

So committed and pushed #6 to 7.x. However, marking back to "needs work" though, because we need to replace the code that was there with something to deal with the permission table cruft this is going to create. IMO this is still critical, or major at best.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: user_update_7006() is broken » 'module' column of role_permission table is not populated on upgrades
catch’s picture

catch
he/him
Primary language English
commented

So we can try to do this in module_enable(), but if we do that, we'll need do deal with:

/**
 * Enable field and field_ui modules.
 */
function system_update_7020() {
  $module_list = array('field_sql_storage', 'field', 'field_ui');
  module_enable($module_list, FALSE);
}

Since module_enable() invokes (lots of) hooks, we should probably not use that at all. This might get fixed when #1239370: Module updates of new modules in core are not executed is dealt with.

chx’s picture

chx commented

I am not too convinced a little DB cruft mandates a critical/major.

chx’s picture

chx commented
Status: Needs work » Closed (won't fix)

I am not even convinced this is a real issue.

chx’s picture

chx commented
Priority: Critical » Minor
Status: Closed (won't fix) » Fixed

http://drupal.org/node/1234834#comment-4869640 that one made this actually fixed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Active

afaik this is a real issue. Try uninstalling then reinstalling a module with the admin role (I have not tried this myself recently).

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Active » Needs review
Issue tags: -Upgrade path, -D7 upgrade path

#15: 1234834_defined.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Upgrade path, +D7 upgrade path

The last submitted patch, 1234834_defined.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs issue summary update

I think the patch in #15 is not the correct one; webchick says in #26 that she committed #6.

Tagging for a summary update to sort the problem out.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Reviewed & tested by the community

I have tested this according to #32 and could not reproduce the issue anymore. This is how I've approached this:

- Install a brand new D7.x-dev
- Create a user admin with the admin role assigned to it
- Go to admin/modules and uninstall and reinstall the module dashboard => no error occured anymore

As to webchicks comment #26 I ran the test again and before I removed 'dashboard' from the modules column in the role_permission table.

Again, no error occured. So I put this on RTBC

As to xjms comment #35 I can confirm that the patch in #6 got committed.

David_Rothstein’s picture

David_Rothstein commented
Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs work

I don't see any patch to commit here... adjusting issue status.

I'm not sure what the problem with the admin role was, but the essential issue (e.g. as described in #26) still exists. In particular, you can do something like:

  1. Install the Statistics module in Drupal 6 and assign some of its permissions.
  2. Upgrade to Drupal 7.
  3. Disable and uninstall the Statistics module and notice that the permissions are still left behind in the database.
  4. Re-enable the Statistics module and notice that the old permission assignments are suddenly active again.

This is contrary to how things normally work (uninstalling a module should remove all its permission assignments and leave you with a blank slate if you later turn it on again) so there's definitely a bug still to fix here; how high-priority it should be I'm not sure.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.