Closed (fixed)
Project:
Role Delegation
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
21 Mar 2017 at 06:26 UTC
Updated:
17 May 2019 at 16:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
benjy commentedComment #3
sophie.skThe patch works really nicely and applied cleanly. It's a bit untidy (think there could be a cleaner way of doing it) and still needs tests, though, so marking as needs work.
Comment #4
benjy commentedCan you put forward the cleaner approach?
Comment #5
sophie.skAnything that doesn't include preg_match and sprintf :D
It makes sense and it works, it just looks a little messy, especially without any comments. (Plus line length, etc.)
If I get a chance at some point, I can take a look at a "cleaner version". We have a sprint day coming up in my org and I can look at it then if not before.
Comment #6
benjy commentedAnother solution could be to use use a entity hook since the views bulk edit forms use the config entities not the plugins and filter them out at that point based on access control. We might not need the regex then if we have access to the base plugin Id, the reason we need the regex here is because the config entities are derived automatically by core, 1 per role so the ID isn't known upfront.
I don't see any issues with sprintf() it's an acceptable way to format strings and how this module generates the permissions to begin with.
Comment #7
sophie.skIt's not that there are issues with sprintf/preg_match - like I say it works (and it's valid PHP etc) - it just looks a little messy to my eyes :) Personal preference as opposed to "you're wrong".
Entity hooks sound interesting. Like I say, if I get the time I'll explore the possibility. In the meantime, I'm happy with the patch you provided and it works, so IMO there's no urgency to rewrite something.
Comment #8
benjy commentedAdded a empty check in case options is empty.
Comment #9
berdirroleassign has a nicer fix for this: http://cgit.drupalcode.org/roleassign/tree/src/Plugin/views/field/RoleAs...
Although I think it would be better as a plugin alter hook, this isn't optional.
Comment #10
berdirSee also the views.inc file that uses that for the user bulk "field".
Comment #11
artem0793 commentedComment #12
artem0793 commentedComment #13
artem0793 commentedComment #14
sophie.skLatest patch works well, but I noticed there are a couple of spelling mistakes eg $plugin_defenition so marking as needs work.
Comment #15
laughnan@artem0793 - This patch worked like a dream!
@Sophie.SK - I've updated the patch to fix the spelling errors. Can you re-review and make this as RTBC if it's ok?
Comment #16
laughnanComment #17
sophie.skLooks good to me :) And it works like a dream.
Comment #18
heddnIt looks like all feedback has been addressed, including #9. Are there any other blockers to this landing?
Comment #19
aaronbaumanBump. Can we get this committed?
Comment #20
jeroentBump.
Comment #22
jeroentCommitted and pushed to 8.x-1.x. Thanks!
Comment #23
jeroentComment #25
jeroent