I think there is a case where one could argue that the project object should be passed as an argument to hook_project_permission_info() so that permissions can be made available contextually for each project. The use case of this is checking that a project has the relevant features enabled before displaying permissions for those features. For example some project on drupal.org don't have repositories associated with them, such as http://drupal.org/project/webmasters. The way that versioncontrol_project will attempt to save these permissions will fail if no repo is associated with a project, and I feel it'd be cleaner to not show the permissions if they won't be saved.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 1003552-project_permissions_context-7.patch | 6.35 KB | mikey_p |
| #5 | 1003552-project_permissions_context-2.patch | 4.86 KB | mikey_p |
| #2 | 1003552-project_permissions_context-2.patch | 4.86 KB | mikey_p |
Comments
Comment #1
mikey_p commentedtagging
Comment #2
mikey_p commentedHere's a first attempt at this for project itself, and project_release module.
Comment #3
mikey_p commentedHere's a first attempt at this for project itself, and project_release module.
Comment #4
mikey_p commentedsettings to needs review.
Comment #5
mikey_p commentedGuess the patch failed to attach.
Comment #6
dwwYeah, this makes sense, and I agree this is a worthy addition to the API. However, it's unfortunate we have to call this from places that don't even have a real project $node yet. Not sure there's much we can do about that. Anyway, reviewing and testing, here's what I've found:
A) The project_release_project_permission_info() hunk should either use
$project = NULLas the arg or not exist at all. I think PHP does something sane if you invoke a function with more arguments than its expecting, but I'm not sure. Might be cleaner to add it (but it should default to NULL).B) If we're going to change project_release like this, we should probably have a patch for project_issue.
C) This is missing a patch against project.api.php (for both hook_project_permission_info() and _alter()).
D) The caching in project_permission_load() is all fubar now. If the results of the info() and alter() hook can vary per project, we need to use the project nid as the cache id for our static cache. Basically, we need an array of cached info arrays indexed by nid.
Comment #7
mikey_p commentedThanks, I was planning on rolling the hunk for project_release later, this was just enough to get some feedback. You're definitely right that it doesn't complain about extra args, only missing ones.
A) Fixed this
B) Opened #1005736: Update hook_project_permission_info() to accept project agrument
C) Fixed
D) Fixed
I also ran tests, and they all passed, and ran through the patch at #781344: Extend project maintainer UI for manipulating git project ACLs manually.
Comment #8
dwwYup, perfect. Ship it!
Comment #9
mikey_p commentedCommitted.