Return partially populated OgRole entities when gathering default roles #237
Conversation
…le, and the role name. Fixes #216.
| public function getName() { | ||
| // If the name is not set yet, try to derive it from the ID. | ||
| if (empty($this->name) && !empty($this->id())) { | ||
| list(, , $name) = explode('-', $this->id()); |
There was a problem hiding this comment.
This seems slightly limited. Maybe it would be better to explode, but pop off the last element and use that?
There was a problem hiding this comment.
There should always be three elements in the current implementation. But I think you're on to something, this is indeed a bug. I don't think there is anything stopping people to use dashes in their entity type IDs and bundle IDs, which means that counting the dashes is not going to cut it.
I'll change it to match exactly "{$this->getGroupEntityTypeId()}-{$this->getGroupBundleId()}-$role_name", then we'll eliminate any dash-related problems.
|
Addressed everything. Now that the If tests come back green this is ready for a fresh look. |
| @@ -187,6 +222,10 @@ public function save() { | |||
| throw new ConfigValueException('The group bundle can not be empty.'); | |||
| } | |||
|
|
|||
| if (empty($this->getName())) { | |||
| throw new ConfigValueException('The role name can not be empty.'); | |||
There was a problem hiding this comment.
we seem to have a similar check in validate, however that one throws a different exception. Maybe consolidate it, and call validate() from here instead?
There was a problem hiding this comment.
I had a look at this but it is not really possible I think. The DefaultRoleEvent::validate() method is used to validate only the data that is necessary for the event listener to gather the default roles. This one is in OgRole::save() and validates everything required to successfully store the entity.
These are different use cases, and calling validate() from here will make OgRole depend on DefaultRoleEvent.
|
@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! |
…oupManager::getDefaultRoles().
|
Addressed the remarks, ready for review! |
|
Wonderful, thank you |
In #217 (comment) @amitaibu suggested that we should return partially populated
OgRoleentities 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.