apeisa / UserGroups

Adds users groups and page based permission to ProcessWire
GNU General Public License v2.0
11 stars 4 forks source link

Proposal to use hooks directly on User::has*Permission #8

Closed niklaka closed 10 years ago

niklaka commented 10 years ago

Didn't want to make such a big change without bringing it up for discussion first so I pushed the most recent changes to a new branch (automatic-permissions).

Current master requires users to have a role with page-edit permission for them to be able to actually edit pages they've got edit access to through groups. Trying to get around this I ended up hooking User::hasPagePermission() and it did work quite nicely. Then I took a closer look on viewable()/editable()/etc in PagePermission.module and came to a conclusion we shouldn't be hooking them at all but instead only those has*Permission() methods in User.

PagePermission's methods define logic on a higher level than what UserGroups is trying to modify; hooking those we would (we did) end up copying code for handling unpublished pages, pages without template files etc. Instead, the part we do want to modify is where page permissions are defined: in page tree instead of templates. By hooking that level directly we get complete control on how it's determined whether user has access somewhere or not.

This also gives us the possibility to easily manage "add & edit children" discussed in issue #7 (or anything else for that matter) in a way that integrates seamlessly to the PW core. Just waiting for Ryan to make those two methods hookable (ryancramerdesign/ProcessWire#328).

Should I just merge the changes to master or are there some downsides to this I haven't noticed?

apeisa commented 10 years ago

I agree. Let's wait Ryan's approval that those hooks are ok before merging. If it's good to go, then I see no problems going further this route.

niklaka commented 10 years ago

This one was merged earlier today.