I've been asked to rewrite OGUR, mainly for performance reasons, to only contain the essential functionality:
1) Allow group admins to assign additional roles for each individual member of a single group. Those additional roles are determined and assigned by the requested page (e.g. node/123 belonging to group XYZ for which the user was granted additional roles).
2) Additional node access permissions are granted to users within the context of a group. Those additional permissions only apply to nodes belonging to the same context (group audience).
The current module is a really nice performance killer and contains a bloat of configuration options and integration code, completely unrelated to the actual functionality. (scope creep)
So, this might sound harsh, but I need to know quickly: Would you accept me as co-maintainer, creating a new DRUPAL-6--2 branch, and ripping off all unrelated code (if required, into a new sub-module, e.g. og_user_roles_bonus)?
If not, please mark this issue won't fix and I'll go ahead and create a new module/project.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | og_user_roles-HEAD.init-docs.patch | 2.25 KB | sun |
| #11 | og_user_roles-HEAD.patch | 103.55 KB | sun |
| #10 | og_user_roles-HEAD.fix_.patch | 5.85 KB | sun |
| #9 | og_user_roles-HEAD.php-notices.patch | 3.97 KB | sun |
| #8 | og_user_roles-HEAD.cleanup-manual.patch | 88.82 KB | sun |
Comments
Comment #1
somebodysysop commentedSounds like a great idea. Would you mind sending me an email?
Comment #2
somebodysysop commentedYou said you were in a rush. Waiting to hear something. I take it you're pretty experienced at this, so just a note as to how you see this co-maintenance working.
Comment #3
sunCross-posting from my mail...
These are steps I imagine:
1) Create a new 2.x branch.
2) Code clean-up, primarily using coder_format, i.e. coding-style only.
3) Create tests for OGUR's core functionality (as outlined above).
4) Code clean-up, focusing on logic. (primarily for me to understand "hidden" reasonings and identify how add-on/bonus features can be moved away)
5) Major rewrite, removing all bonus features.
6) Major rewrite, overhauling OGUR's core functionality.
7) If required, re-add bonus features in a new sub-module.
Comment #4
somebodysysop commentedSounds like a good plan. Let's do it!
Comment #5
muldos commentedHello there !
This module is also very usefull for my current project, and I need to decide if i will wait for the upcoming 2.0 or making my own "OGUR".
I'd like you to clarify the point #2 :
2) Additional node access permissions are granted to users within the context of a group. Those additional permissions only apply to nodes belonging to the same context (group audience).
For exemple let's say that I used a panels3 page with an uri of sample-uri/%node/title-here.
where %node will be the nid of a group type node.
With the current OGUR I am able to load the roles of a user in this group by implementing the newly created hook_og_user_roles_gid.
So panes on this page can rely on permissions granted for the group node to the current logged in user.
Will this feature be kept in the upcoming "cleaned" 2.0 module ?
I think that preserving this hook which allow to make a good integration between OGUR and panels(or any custom module which add custom menu_path) would be great.
David
Comment #6
sunSorry for the delay. I'll start this week.
Comment #7
sunI've taken over HEAD. Will post incremental patches here.
Starting with an automated clean-up using my fellow coder_format script.
Comment #8
sunManual coding-style clean-up.
Comment #9
sunFixed PHP notices.
Comment #10
sunFurther fixes. Note that 6.x-1.x doesn't seem to work at all currently. I already compared the code to 6.x-1.0 (which is the last version that I know was working), but could not find any code that could have caused this.
Well. I'll re-implement this anyway.
Comment #11
sun91 passes, 0 fails, and 0 exceptions
Major rewrite accomplished and working in HEAD now. I'll roll out this on a staging site today.
The upgrade path (and updates) probably need some tweaking though. We have ~50 less variables now, which need to be removed.
By speaking of removed: All features that are not directly tied to the base functionality have been removed. See CHANGELOG.txt and/or commit log for details.
Note that the implementation is still a hack, but on a much lower level. I don't expect you to understand the details of the implementation. ;)
Attached patch is the final (incremental) implementation and last clean-up only. Posting this for reference only. All incremental steps can be found in the CVS history.
I'll tweak the release nodes a bit now, so we get a (not publicly visible) dev snapshot of 6.x-2.x in approx. 12 hours.
Comment #12
sunThough you can ask, of course. Also, I've extensively documented all implementation details in PHPDoc.
Comment #13
sunWe should rewrite the project page as well. Not sure if I'm allowed to. (I'd probably nuke everything there and start with a clean description from scratch)
Comment #14
sunHere we go: http://drupal.org/node/472072
@SomebodySysop: Are you still there? Any thoughts/feedback?
Comment #15
Equ commentedHi, sun!
Looks like some work is going on here. May I ask you a question? I need OGUR functionality on my website, I want people signing up or creating groups automatically be assigned to a certain role (so they can create let's say "article" content type only in their own group).
Can I use 6.x-2.x branch of module on my website? If yes, when approximately can we see it in official release?
Thanks!
Comment #16
sun@Equ: You are in luck. Assigning a default role for regular members and/or a default role for group admins are the only two additional features that remained from 1.x.
2.x was successfully deployed in the meantime, so all I can do currently is to recommend you to test it and report back the issues you encounter.
For an official release, I'm still waiting for SomebodySysop to provide some feedback, especially about the removed features from 1.x. The original proposal was to optionally move them into a sub-module (although they would better suit into a completely separate module, since they have nothing in common with user roles) and they are simply removed as of now. Users upgrading from 1.x will lose that functionality. So if SomebodySysop wants to keep them in any way, we need to alter the upgrade path before releasing 2.0.
Comment #17
Equ commentedsun,
Thanks for the good news, I'll be testing the latest dev release in couple of days.
Comment #18
Equ commentedI was testing the latest dev release on the local server and everything seems to be working really good :-)
Comment #19
sunI just realized that there is/was a 5.x-3.x branch, which is probably feature-wise identically to 6.x-1.x. I wonder whether we should use 4.x as major version for this new, rewritten version then. Otherwise, users from 6.x-1.x or 5.x-3.x might be confused about the major version numbers, because 2.x is in the middle of both. 4.x would clearly mark a difference to all previous releases.
Comment #20
sun@Equ: Your feedback (as a OGUR user) on all raised questions/issues since comment #11 would be very appreciated as well! In short:
a) What do we do with the project page?
b) How do we tackle the removed features of 1.x?
c) Which major version should we use?
Comment #21
Equ commented@sun: here what I think
a) I'd rewrite project page from scratch leaving their some info about old module with links to new sub-modules/separate modules
b) If possible/needed group some features and move them into one sub-module, other features move into separate modules
c) I like your idea about 4.x, it makes sense, so I'd stick with it.
Comment #22
Equ commented@sun: Do you think I can start using 6.x-2.x dev release on my working website? Can't wait for that.
Comment #23
sun@Equ: Yes, I think so. At least, I already deployed it on a large-scale production site. :)
Comment #24
amitaibu@sun,
Very cool. I haven't tested yet, but from looking at the code - can you please explain why you did the fork og_user_roles_determine_context()?
> OG invokes menu_get_item() in og_determine_context() via og_init(), which
performs the very first access check for the current path and is irreversible
(statically cached)
Maybe instead of the fork, you can use og_determine_context() and later menu_set_item() to set the access according to OGUR's logic.
Comment #25
sunHah. Awesome! Thanks for reviewing.
Does attached patch explain it better?
Comment #26
somebodysysop commentedI've had a chance to at least look at the new code. It's going to take a minute to figure out how to create a sub-module of the current features that are removed from it. So, I would say move forward with a 6.x-2.x release for new users as well as existing users who do not want anything other than the base functionality. There should be some upgrade path for this latter group of users.
Split it in half, clearly indicating what 6.x-2.x is at the top, separated by a line then a a brief summary of 6.x-1.x with a link to a document page to provide most of the details that are currently on the page.
People who need them stay with 6.x-1.x until a 6.x-2.x compatible module that contains them is available. We will have a very good idea of just how much they are wanted/needed after the dust settles and we see the usage statistics.
Not sure what this means.
Comment #27
amitaibu@sun,
I have tried and now I understand better - there's no $reset option for menu_get_item().I should have understood it from your drupal_static_reset comment.
One tiny thing in ogur.pages.inc - I'll open an issue for it.
Oh, and there are 0 bugs in the views handlers ;)
Comment #28
sunThanks!
@Amitaibu: For now, no-brainers like the core user role constant one, I'd prefer to keep in this issue here - since I cannot easily subscribe to all 2.x issues.
Also, I'm not sure to which views handlers you are referring. ;)
@SomebodySysop: Thanks for your feedback!
Your proposed solution to a) and b) makes sense and would work for me. I will try to spare some time for the project page rewrite as soon as possible.
Regarding the major version number, see my follow-up in #19. I think we should go with 4.x as major version, to not confuse any users (either coming from Drupal 5 or 6).
Comment #29
sunI have updated the release node to use the proper major version now.
I'll go ahead and create a handbook page now.
Comment #30
sunI have updated the project page, splitted it into two sections, but also moved the lower section into the handbooks.
I will create a first 4.0 release now.
Comment #31
sun"copied" I meant. I'm leaving the decision about when to remove the old (quite lengthy) explanations to SomebodySysop.
btw, using 4.x as major version turned out to be a very good decision: The handbook page is totally clear now: http://drupal.org/node/163565
Comment #32
somebodysysop commentedThanks, Sun, for the changes. This is more or less what I had in mind. Since you've copied everything to the handbook page, and clearly explain the that there are separate versions of the module, I'd say it's safe to remove the old stuff from the project page, and they have been removed.
Comment #33
glass.dimly commentedThanks for all the work, folks.
At this point, the project page is in need of a good stiff edit, as it's quite confusing. The project page refers to the handbook and the handbook refers to project page. Infinite loop. And the handbook page doesn't explain the difference between 2x and 4x like this thread does.
Note the contradiction about "the following" and "the below." Another thing, it's unclear to the causal reader what features are bonus or not.
I'd also like to know if the 4x branch supports OG 6.x.2 or 6.x.1.
Comment #34
sunI've added a major version info to the Organic Groups dependency information on the project page. You need at least the latest official release of OG 2.x due to API changes since, well, actually 1.3, but explaining that you can use 1.3 but not 1.2 would be even more confusing. So just use 2.x. ;)
I don't understand the documentation issue though. The project page states:
That linked handbook page states:
I've tried to clarify the project page now. Since everyone can edit handbook pages, could you clarify the handbook page to something that would make sense for you?
Comment #35
glass.dimly commentedYep, that makes more sense. I've updated the handbook page with more information from the discussion that happened at this thread and added them to the handbook page here: http://drupal.org/node/163565
Comment #36
glass.dimly commented@sun,
Thanks for updating the project page. I've got a few more things that I'd really like to see on that project page, that would be very helpful for figuring out which branch of the module to install for the masses.
1. Does the 2.x version of the OGUR module depend on OG 2.x?
2. It would be very useful for people considering installing the 4.x module to have a list of the "bonus" features that were removed so the issue queue doesn't fill up with lots of "OG 4.x broke my site!" threads. I'm lookin' out for you, sun. ;^)
Comment #37
recrit commentedsun, thanks for the rewrite! this is my first time using OGUR and reading all the bloated features in the previous versions, i'm glad I started with 4.x.
I found a small error on line 344, too small to start a new issue.
$rid never gets set in the function og_user_roles_og; therefore, when og_user_roles_role_delete gets called it sends $rid == null which removes all roles from the user. My understanding of this line, $rid should be $default_admin_role.
Comment #38
sun@glass.dimly: Thanks! You may find the list of all removed features in CHANGELOG.txt resp. the release node for 4.0: http://drupal.org/node/493572 - admittedly, those do not explain much, but the reality is also that I don't even know what features I actually removed, because all I knew was that the code did not relate to any core feature of the module. ;)
@recrit: Thanks, that's true. Yeah, similar to the corresponding "add" function right above that line, it should use $default_admin_role. Committed this fix.