Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
See #773828: Upgrade of disabled modules fails for the background and more details of the bug - it's because it calls user_role_grant_permissions() which does not work on a disabled module's permissions.
Options to fix this include:
- Fix #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module) (may or may not be possible/simple).
- Use a direct database query against the {role_permission} table here, which would mean that any module which assigns permissions in one of its update functions would need to do the same thing.
Comment | File | Size | Author |
---|---|---|---|
#19 | 794184-contact-19.patch | 1.41 KB | catch |
#10 | 794184-contact.patch | 831 bytes | catch |
#7 | 794184_user_role_grant_permission.patch | 1.56 KB | JacobSingh |
#5 | 794184_user_role_grant_permission.patch | 1.55 KB | JacobSingh |
#2 | 794184-2.patch | 1.21 KB | chx |
Comments
Comment #1
Gábor HojtsyI 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.
Comment #2
chx CreditAttribution: chx commentedComment #3
JacobSingh CreditAttribution: JacobSingh commentedI 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.
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
Comment #4
chx CreditAttribution: chx commentedso i'd rather not add a separate func
Comment #5
JacobSingh CreditAttribution: JacobSingh commentedIf 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.
Comment #7
JacobSingh CreditAttribution: JacobSingh commentedSorry, posted that while holding a screaming baby.
This one actually should work should anyone think it is a good idea (not attached personally).
Comment #8
Dries CreditAttribution: Dries commentedAbout 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedJacob already moved his patch over to #737816: user_role_grant_permissions() throws PDOException when used with non-existent permissions (e.g. a permission for a disabled/uninstalled module) and there is some discussion of @chx's approach there also. I think it would be better if we left the general API issue/discussion in that issue, where it's already ongoing.
I mainly opened up this issue so we could track the critical contact.module bug itself, and potentially commit a hack to fix it (e.g. just do the direct database query for now) if it winds up being a final blocker for the D6->D7 upgrade path, for example.
Comment #10
catchDirect query. The other issue hasn't been touched for three weeks now.
Comment #12
aspilicious CreditAttribution: aspilicious commented#10: 794184-contact.patch queued for re-testing.
Comment #14
aspilicious CreditAttribution: aspilicious commentedlanguage code has nothing to do with this :s?
retest?
Comment #15
aspilicious CreditAttribution: aspilicious commented#10: 794184-contact.patch queued for re-testing.
Comment #17
catch#10: 794184-contact.patch queued for re-testing.
Comment #18
mr.baileysThe 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
Comment #19
catchAlso removed contact_update_dependencies().
Comment #20
mr.baileysApplied and reviewed, and agreed that contact_update_dependencies() is obsolete if we're not using user_role_grant_permissions anyway.
Comment #22
aspilicious CreditAttribution: aspilicious commented#19: 794184-contact-19.patch queued for re-testing.
Comment #24
catch#19: 794184-contact-19.patch queued for re-testing.
Comment #25
aspilicious CreditAttribution: aspilicious commentedBack to rtbc?
Comment #26
webchickIt 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!
Comment #28
Dave ReidActually, 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().
Comment #29
Dave ReidComment #30
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like there's already a patch for that at #939562: Update fails on contact #7002, actually?
Comment #31
Dave ReidGrr for issues not filed under the correct component.