[MAGETWO-54158]:+ Corrected ACL rules for the sendfriend module and s… #9628
Conversation
…everal backend store configuration
|
|
| * @param ModuleDataSetupInterface $setup | ||
| * @return void | ||
| */ | ||
| protected function upgradeAcl(ModuleDataSetupInterface $setup) |
There was a problem hiding this comment.
Is there any reason for this method is protected? Looks like private is more suitable in this case.
| class UpgradeData implements UpgradeDataInterface | ||
| { | ||
|
|
||
| /** |
There was a problem hiding this comment.
{@inheritdoc} is sufficient in this case.
| } | ||
|
|
||
| /** | ||
| * @param ModuleDataSetupInterface $setup |
There was a problem hiding this comment.
Would be nice to add a comment which explains background under this action.
app/code/Magento/Config/etc/acl.xml
Outdated
| @@ -12,16 +12,15 @@ | |||
| <resource id="Magento_Backend::stores"> | |||
| <resource id="Magento_Backend::stores_settings"> | |||
| <resource id="Magento_Config::config" title="Configuration" translate="title" sortOrder="20"> | |||
| <resource id="Magento_Config::advanced" title="Advanced Section" translate="title" sortOrder="90" /> | |||
| <resource id="Magento_Backend::advanced" title="Advanced Section" translate="title" sortOrder="90" /> | |||
There was a problem hiding this comment.
Did you consider introducing these resources in the Backend/etc/acl.xml?
| * @param ModuleDataSetupInterface $setup | ||
| * @return void | ||
| */ | ||
| protected function upgradeAcl(ModuleDataSetupInterface $setup) |
There was a problem hiding this comment.
Please consider making this method private
|
Hi @jspoe |
|
Thank you for your feedback! |
+ Made setup functions private + Moved Backend ACL nodes to Backend module instead of Config module
|
@jspoe Thanks, we will proceed with review shortly. |
|
@jspoe It looks like these changes introduce some unwanted dependencies between the modules in question. We will look into possible solution and let you know how we can fix it. |
|
Yes I understand. It is either part of the Magento_Config module or the Magento_Backend module. Please let me know as soon as possible if I can fix this in a better way for you. |
|
@jspoe Looks like the best solution in this case would be to move upgrade scripts to the specifically created modules, something like SendfriendAcl or SendfriendAuthorization, which would have correct dependencies and would allow other modules to be disabled/removed safely. |
|
Hi @ishakhsuvarov, I agree, and I will pick up the requests myself if possible. Thanks! |
|
@jspoe Thank you |
|
@jspoe I am closing this PR for now due to inactivity. Please feel free to update and reopen whenever you wish to continue work on it. |
Description
Added an acl.xml to the send-friend module
Added upgrade data script for backward compatibility for ACL-roles that were already selected for users.
Changed the acl.xml for the backend module
Added upgrade data script for backward compatibility for ACL-roles that were already selected for users.
Fixed Issues (if relevant)
Manual testing scenarios
Expected result:
All the tabs that were selected in the role should be visible
Contribution checklist