contact_update_7002() fails when the contact module is not enabled

I think that using direct DB queries was the norm before (given no API function to do this), so now that we have the API function, it is probably better to make it work compared to going back to the dark ages :) Looks like a bad maze of issues around this though. Tricky problem.

I haven't tested that this actually resolves the issue, but the code should do what it is intended to do.

Perhaps another approach is to refactor user_role_grant_permissions() so that one could grant a single permission and provide all the variables.

function user_role_grant_permission(int $rid, string $permission, string $module) {
  // run the db_merge here.
}

This would allow update functions to just run on their own, decoupling the module loading piece.

But if we're not doing that, this seems fine.

-J

now that we have the API function, it is probably better to make it work

so i'd rather not add a separate func

If the API function isn't sufficiently decoupled to perform the tasks it is intended for, then adding a new function is just improving the job the API is doing. It's not stepping around it or changing it's behavior.

I already RTBC'd the patch which loads all the module files. This is an okay approach and means that function will always work as advertised. The downside is that it will have to load every module file. AFAIK, we don't load module files for disabled modules, so this is potentially an issue for performance and if a disabled module has runtime code in it or other issues (which is why it was disabled).

Either way is really fine with me, but here is a patch which provides an alternative means of solving the same problem by decoupling the API.

Sorry, posted that while holding a screaming baby.

This one actually should work should anyone think it is a good idea (not attached personally).

About the patch in #7: I don't understand why user_role_grant_permissions() (plural) throws a permission, and why user_role_grant_permission() (singular) doesn't. That seems inconsistent behavior to me. Shouldn't we throw the permission in user_role_grant_permission(), assuming that is what we want to do.

Direct query. The other issue hasn't been touched for three weeks now.

#10: 794184-contact.patch queued for re-testing.

language code has nothing to do with this :s?

retest?

#10: 794184-contact.patch queued for re-testing.

#10: 794184-contact.patch queued for re-testing.

The only thing missing IMO is an inline comment explaining why user_role_grant_permissions() cannot be used, pointing to #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module) (maybe even in @todo-form?)

Other than that this is RTBC

Also removed contact_update_dependencies().

Applied and reviewed, and agreed that contact_update_dependencies() is obsolete if we're not using user_role_grant_permissions anyway.

#19: 794184-contact-19.patch queued for re-testing.

#19: 794184-contact-19.patch queued for re-testing.

Back to rtbc?

It sounds like Dries's comments above no longer apply to this latest patch, which just does a merge query directly. The comment explaining why this is seems self-explanatory to me. I agree with comments above that it'd be nice to resolve this with API function calls, but given this is just an update_XXX function, I'm not really that fussed about it, personally. Dries, feel free to revert if you disagree.

Committed to HEAD. Thanks!

Actually, we do need the dependencies line, because the {role_permission}.module column doesn't exist until user_update_7006() and causes failures. So I have to re-open this to revert the removal of contact_update_dependencies().

Grr for issues not filed under the correct component.