add inGroup(s) methods to Group #7046

andrewandante · 2017-06-21T15:15:35Z

No description provided.

tractorcow · 2017-06-26T05:32:38Z

src/Security/Group.php

+     * @param string|int|Group $group
+     * @return bool
+     */
+    public function inGroup($group) {


Would be good if the phpdoc told me what this method did. :)

Is this checking that the provided group is IN the invoked group, or the other way around? Does this work recursively?

Tests would also be nice.

tractorcow · 2017-06-26T05:32:58Z

src/Security/Group.php

+        } elseif ($group instanceof Group) {
+            $groupCheckObj = $group;
+        } else {
+            user_error('Member::inGroup(): Wrong format for $group parameter', E_USER_ERROR);


InvalidArgumentException is preferred response in this case.

tractorcow · 2017-06-26T05:33:21Z

src/Security/Group.php

+            user_error('Member::inGroup(): Wrong format for $group parameter', E_USER_ERROR);
+        }
+
+        if(!$groupCheckObj) return false;


psr-2 will fail here. All conditionals need { }

tractorcow · 2017-06-26T05:34:06Z

src/Security/Group.php

+            $groupCheckObj = Group::get()->byID($group);
+        } elseif (is_string($group)) {
+            $SQL_group = Convert::raw2sql($group);
+            $groupCheckObj = Group::get_one("\"Code\" = '{$SQL_group}'");


Please use ORM filter API instead of raw SQL please.

andrewandante · 2017-06-26T14:36:14Z

Tests added, and other feedback addressed @tractorcow - thanks for taking the time to look at it

tractorcow · 2017-06-27T04:13:36Z

src/Security/Group.php

+            $groupCheckObj = Group::get()->byID($group);
+        } elseif (is_string($group)) {
+            $SQL_group = Convert::raw2sql($group);
+            $groupCheckObj = Group::get()->filter(['Code' => $SQL_group])->first();


Don't raw2sql when using filter api; It'll double encode.

Group::get()->filter('Code', $group)->first();

is all you need

tractorcow · 2017-06-27T04:14:50Z

src/Security/Group.php

+     * @param string|int|Group $group Group instance, Group Code or ID
+     * @return bool Returns TRUE if the Group is a child of the given group, otherwise FALSE
+     */
+    public function inGroup($group)


Edge case, is $group->inGroup($group) ?

I think it should return true. Added a test.

tractorcow · 2017-06-27T04:19:51Z

src/Security/Group.php

+     * Check if the group is a child of the given groups or any parent groups
+     *
+     * @param array $groups
+     * @param bool $strict set to TRUE if must be in ALL groups, or FALSE if must be in ANY


Not a well named arg; $strict means something other than what you're using it for. Did you mean to flag as conjunctive (and) or disjunctive (or)? Perhaps $conjunctive = false is better?

tractorcow · 2017-06-27T04:20:17Z

src/Security/Group.php

+     * @param bool $strict set to TRUE if must be in ALL groups, or FALSE if must be in ANY
+     * @return bool Returns TRUE if the Group is a child of any of the given groups, otherwise FALSE
+     */
+    public function inGroups($groups, $strict = false)


Edge case: is $group->inGroups([]); true? Does it depend on whether the search is conjunctive or disjunctive?

tractorcow · 2017-06-27T04:23:48Z

src/Security/Group.php

+    public function inGroups($groups, $strict = false)
+    {
+        foreach ($groups as $group) {
+            if ($this->inGroup($group)) {


you will be calling $this->collateAncestorIDs() once for each loop. Why not inline inGroup() into this method and optimise it a little better? You could create a set of all groupIDs, and another set of collateAncestorIDs(), then do subset comparison (array_intersect). You'd just need to count the intersection to determine if inGroups is true (intersect > 0 for disjunctive search, intersect === total for conjunctive search).

Have reworked this, I think I understood what you meant? Let me know if I've gone down the wrong rabbithole 🐰

tractorcow · 2017-06-27T22:29:25Z

src/Security/Group.php

+    protected function identifierToGroup($group)
+    {
+        if (is_numeric($group)) {
+            $groupCheckObj = Group::get()->byID($group);


You are converting the id to a group, and then to an id again. That's a very inefficient SQL overhead.

tractorcow · 2017-06-27T22:30:26Z

src/Security/Group.php

+            $candidateIDs[] = $candidate->ID;
+        }
+        if ($requireAll) {
+            return array_intersect($candidateIDs, $ancestorIDs) === $ancestorIDs;


Sorting issues [1, 2] !== [2, 1]. You should only need to measure size of each group.

You should also be comparing to $candidateIDs not to $ancestorIDs, since $ancestorIDs may contain groups that aren't being compared to.

tractorcow · 2017-06-27T22:33:00Z

src/Security/Group.php

+        if (is_numeric($group)) {
+            $groupCheckObj = Group::get()->byID($group);
+        } elseif (is_string($group)) {
+            $groupCheckObj = Group::get()->filter('Code', $group)->first();


If you require a group somecode that doesn't exist, your code will silently ignore it and pass.

dhensby · 2017-06-28T19:04:50Z

tests/php/Security/GroupTest.php

+    public function testGroupInGroupExceptionForFakeGroupID()
+    {
+        $group = new Group();
+        $group->inGroup(99999999);


I'm not sure an invalid argument exception should be thrown when a correct type is passed

I think it should be an exception, but the wrong exception message is thrown. It says "invalid type" when the type is valid, but the ID isn't a real record.

I've updated the exception to cover all the bases - i.e. must be $correctType and exist

tractorcow · 2017-06-28T21:53:22Z

src/Security/Group.php

+     */
+    protected function identifierToGroupID($group)
+    {
+        if (is_numeric($group) && Group::get()->byID($group)) {


Is the extra query needed here?

maybe $group->inGroup(999999); should just be false in this case, rather than an error, to save a whole query.

My earlier comment about checking that the group exists was intended only to check the result of a query if you had already performed it, since there's no new overhead anyway.

tractorcow · 2017-06-28T21:53:56Z

src/Security/Group.php

+        if (is_numeric($group) && Group::get()->byID($group)) {
+            return $group;
+        } elseif (is_string($group) && Group::get()->filter('Code', $group)->first()) {
+            return Group::get()->filter('Code', $group)->first()->ID;


You are running this entire SQL query twice? Can you not put this into a temp variable.

dhensby

I think trying to have a custom error message for every type of invalid argument that's sent in is overkill. It'll be less logic and code just to use a generic message for the exceptions.

Lets keep code complexity down

dhensby · 2017-06-29T11:26:16Z

src/Security/Group.php

+        if ($group) {
+            return $group->ID;
+        }
+        if ($groupID instanceof DataObject) {


This fails to catch objects that don't extend from DataObject

tractorcow

You've been diligent keeping up with this PR so I'm going to be diligent with review.

tractorcow · 2017-06-29T21:48:07Z

src/Security/Group.php

+        foreach ($groups as $group) {
+            try {
+                $candidateIDs[] = $this->identifierToGroupID($group);
+            } catch (\InvalidArgumentException $e) {


If you're catching and returning false, then it's not really an error. Exceptions add alot of overhead (processing time) and shouldn't be used unless it's a real error.

Going back to @dhensby 's feedback

I think trying to have a custom error message for every type of invalid argument that's sent in is overkill. It'll be less logic and code just to use a generic message for the exceptions.
Lets keep code complexity down

It's too hard to get this right just drop exceptions and handle the logic here. If it's $requireAll and null, return false, otherwise if it's null and not $requireAll, don't add it to $candidateIDs. you'll need to move your empty() check to below this loop though, if you could end up with empty $candidateIDs.

tractorcow · 2017-06-29T21:49:54Z

src/Security/Group.php

+            $matches = array_intersect($candidateIDs, $ancestorIDs);
+            return count($candidateIDs) === count($matches);
+        }
+        return count(array_intersect($candidateIDs, $ancestorIDs)) > 0;


same expression repeated above. Why not count($matches) > 0?

dhensby · 2017-06-30T11:38:14Z

src/Security/Group.php

+        if ($requireAll) {
+            return count($candidateIDs) === count($matches);
+        }
+        return count($matches) > 0;


addressed

tractorcow · 2017-07-02T21:20:23Z

Thanks @dhensby and @andrewandante

tractorcow suggested changes Jun 26, 2017

View reviewed changes

andrewandante force-pushed the FEAT/add_inGroup_to_Group branch from 216ef34 to f4a8a74 Compare June 26, 2017 14:45

tractorcow suggested changes Jun 27, 2017

View reviewed changes

andrewandante force-pushed the FEAT/add_inGroup_to_Group branch from 8d85358 to 4395a35 Compare June 28, 2017 16:08

dhensby reviewed Jun 28, 2017

View reviewed changes

tractorcow suggested changes Jun 29, 2017

View reviewed changes

dhensby suggested changes Jun 29, 2017

View reviewed changes

tractorcow previously requested changes Jun 29, 2017

View reviewed changes

dhensby suggested changes Jun 30, 2017

View reviewed changes

add inGroup(s) methods to Group

ab60a16

andrewandante force-pushed the FEAT/add_inGroup_to_Group branch from 69252b1 to ab60a16 Compare June 30, 2017 11:48

dhensby approved these changes Jun 30, 2017

View reviewed changes

dhensby merged commit c69a565 into silverstripe:master Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add inGroup(s) methods to Group #7046

add inGroup(s) methods to Group #7046

andrewandante commented Jun 21, 2017

tractorcow Jun 26, 2017 •

edited

Loading

tractorcow Jun 26, 2017

tractorcow Jun 26, 2017

tractorcow Jun 26, 2017

andrewandante commented Jun 26, 2017

tractorcow Jun 27, 2017

tractorcow Jun 27, 2017

andrewandante Jun 27, 2017

tractorcow Jun 27, 2017

tractorcow Jun 27, 2017 •

edited

Loading

tractorcow Jun 27, 2017

andrewandante Jun 27, 2017

tractorcow Jun 27, 2017

tractorcow Jun 27, 2017

tractorcow Jun 27, 2017

dhensby Jun 28, 2017

tractorcow Jun 28, 2017

andrewandante Jun 29, 2017

tractorcow Jun 28, 2017

tractorcow Jun 28, 2017

dhensby left a comment

dhensby Jun 29, 2017

tractorcow left a comment

tractorcow Jun 29, 2017

tractorcow Jun 29, 2017

tractorcow Jun 29, 2017

dhensby Jun 30, 2017

tractorcow commented Jul 2, 2017

add inGroup(s) methods to Group #7046

add inGroup(s) methods to Group #7046

Conversation

andrewandante commented Jun 21, 2017

tractorcow Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewandante commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Jul 2, 2017

tractorcow Jun 26, 2017 •

edited

Loading

tractorcow Jun 27, 2017 •

edited

Loading