-
Notifications
You must be signed in to change notification settings - Fork 293
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
Move module sharing settings capability filtering to REST controller #5259
Move module sharing settings capability filtering to REST controller #5259
Conversation
|
||
return $this->set( $settings ); | ||
return $this->set( array_merge( $settings, $updated ) ); |
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.
We can't just merge the original settings with updated ones. The problem here is that updated settings may not contain certain subkeys if the user doesn't have some of required capabilities which will lead to a removal of missing subkeys in the updated settings from the original settings.
For example, let's assume we get the following updated settings:
$updated = array(
'moduleA' => array(
'management' => 'owner',
)
)
As you can see there, the moduleA
lacks the sharedRoles
property which has been unset in the Collection_Key_Cap_Filter
class because the user doesn't have the MANAGE_MODULE_SHARING_OPTIONS
capability.
Now if we have the following settings, then sharedRoles
will be removed after merging both arrays.
$settings = array(
'moduleA' => array(
'management' => 'all_admins',
'sharedRoles' => array( 'role1', 'role2' ),
)
)
We need to preserve the current value of the sharedRoles
property for the moduleA
.
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.
LGTM, nice one @aaemnnosttv & @kuasha420 !
* | ||
* @return array Merged array. | ||
*/ | ||
private function array_merge_deep( $array1, $array2, $depth = 1 ) { |
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.
Might be nice to extract this to a shared utility method, this can wait though...
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.
I remember having a similar discussion previously and colocating it in the consuming class was the consensus unless the util is being used in multiple place. So I followed that. We should definitely relocate it to a shared util class like the BC_Functions
one if/when this gets used elsewhere.
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 pointing that out @kuasha420, it's a fair comment!
Also - apologies, as I didn't spot that you had worked on this too. I saw Evan had created the PR so gave him the shout out on my approval message, but hadn't spotted your commits so neglected to tag you too. It's a small thing, but I am glad to have now amended! :)
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist