amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

Return partially populated OgRole entities when gathering default roles #237

Closed pfrenssen closed 8 years ago

pfrenssen commented 8 years ago

In https://github.com/amitaibu/og/pull/217#issuecomment-225639354 @amitaibu suggested that we should return partially populated OgRole entities instead of dumb arrays containing role properties when retrieving the default roles that are created when saving a new group.

As a side effect this also fixes #232 and #216 and obsoletes #235.

pfrenssen commented 8 years ago

Addressed everything.

Now that the OgRole entities are created in the event subscriber I was able to remove a lot of the complexity from the event listener, just as @amitaibu predicted. A very nice bonus is also that we no longer need to inject the OgRole entity storage in the listener, meaning it doesn't need to be a service any more, meaning that the ugly ::reset() method could be removed!

If tests come back green this is ready for a fresh look.

amitaibu commented 8 years ago

@pfrenssen , the recent changes have indeed cleaned up a lot, and it's much easier to follow the logic.

I've added a few comments, mostly about naming. Thanks!

pfrenssen commented 8 years ago

Addressed the remarks, ready for review!

amitaibu commented 8 years ago

Wonderful, thank you