[WIP] Start converting hook_node_access to hook_entity_access for 8.x#38
[WIP] Start converting hook_node_access to hook_entity_access for 8.x#38damiankloip wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
ping @RoySegall will your #35 PR help with these functions to get user roles?
There was a problem hiding this comment.
tl;dr - Yes.
long - I think if we have api function for get the roles, for example, they need to be as a static method on the user role entity. Look me to more encapsulate.
damiankloip
force-pushed
the
entity-access
branch
2 times, most recently
from
3dd3042 to
c47187c
Compare
There was a problem hiding this comment.
This is the main part I need to convert now to move on with this PR. I also started on #40 but that also needs some of this access stuff... :/
There was a problem hiding this comment.
I would take it in several steps. We can start converting the access and get the OG roles/ OG permissions in a second step - otherwise, it's going to be a big one. agree?
There was a problem hiding this comment.
Yes, definitely agree! We need to at least get some chunks in so we can then spin up some smaller PRs around that. Should we comment this out for now maybe with a @todo ?
There was a problem hiding this comment.
A todo and an Issue would be great
|
You are a brave man! This is a big one, and it will need also the tests ported. The hardest part would be to try and keep this PR small without tons of scope creeps :) |
|
Ha :) trouble is there are a bunch of other smaller parts that this needs
|
|
@damiankloip mind if I help with this PR? I'd like to start porting some tests |
There was a problem hiding this comment.
Do we want to move this to own plugin instead of the alter? (Damien, do you have a rule of thumb for when alter VS plugins?)
There was a problem hiding this comment.
I think this is good as an alter hook still. It is not something you would want multiple instances of etc.. Plugins get a bit weird when used outside their usual patterns. Just look at the element plugins in core.... :)
I will convert that to use the module handler directly though.
There was a problem hiding this comment.
We should consider using event more in Og instead too. Maybe we can talk about that next week.
damiankloip
force-pushed
the
entity-access
branch
from
c47187c to
6a844f8
Compare
|
@amitaibu no, you can sure help with this. If you are working on tests that should be nice and separate to my work here. Or did you want to take this over or something? |
|
Or we can get this to a good state for you to take over in a new PR? Just trying to think what's easiest for both of us here :) You can choose. If you wanted to branch from this and take this over, that's cool. If you did I would just focus on #40 more. |
Sounds good, I'll assign to myself. I'll try to tackle on Sunday. |
|
This looks like the test for it: ec-europa@a8b1201 - it mentions it is testing You can merge this commit in this branch. Let me know if you've done so then I can remove the branch in my fork, to avoid duplicate work to be done. |
|
Nice, handy link :) |
|
Github magic :D |
|
Closing in favor of #51 |
4 participants
Didn't get a chance to finish this atm, but pulled the thread that is hook_node_access > hook_entity_access :)