-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IDP-1176] Add groups_external
field to User
#352
[IDP-1176] Add groups_external
field to User
#352
Conversation
Fixing the bug (revealed by the tests) currently... |
PHP's `explode()` returns an array with an empty string if the given string is empty, but we do not want to add that to the `members` list.
Quality Gate passedIssues Measures |
Fixed. |
$this->addColumn( | ||
'{{user}}', | ||
'groups_external', | ||
$this->string()->notNull()->defaultValue('')->after('groups') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Yii syntax is ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP syntax is ugly. 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference between internal and external groups is unclear to me, but I wouldn't be surprised if it is somehow related to a Google API somewhere.
Thanks for mentioning that. The normal list of |
'member' => function (self $model) { | ||
if (!empty($model->groups)) { | ||
$member = explode(',', $model->groups); | ||
} else { | ||
$member = []; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the else
added? Is there some sort of "strict" mode that would flag an error when assigning to a new array key on a yet-undefined variable (e.g. line 581 or 585). If so, then maybe this is better:
'member' => function (self $model) { | |
if (!empty($model->groups)) { | |
$member = explode(',', $model->groups); | |
} else { | |
$member = []; | |
} | |
'member' => function (self $model) { | |
$member = []; | |
if (!empty($model->groups)) { | |
$member = explode(',', $model->groups); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a variable without defining it (e.g. assigning something to an array on an uninitialized array, in PHP) seems like a code smell. I simply added the else
to ensure that the array would exist and be in the desired state, regardless of which path through the logic was executed.
Initializing the array first, as you suggested, is another valid option, but since it would be replaced with a different array within the if-block, I decided to only set it to an empty array if needed, rather than every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for discussing it, though 👍
$this->addColumn( | ||
'{{user}}', | ||
'groups_external', | ||
$this->string()->notNull()->defaultValue('')->after('groups') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP syntax is ugly. 😛
IDP-1176 Add groups_external field to ID Broker's database
Added
groups_external
field to UserChanged (non-breaking)
groups_external
values in a User'smember
list during loginFeature PR Checklist
make composershow
make psr2
Note:
This does not yet provide any mechanism for setting the new
groups_external
field. That will come in a subsequent PR.