Skip to content
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

New feature: UI to list and remove orphaned ACL resources in backend. #3647

Merged
merged 32 commits into from
Feb 16, 2024

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented Nov 12, 2023

Description (*)

Orphaned ACL resources are left behind in the admin_rule table when modules are removed. This PR provides a UI to list these orphaned resources and delete them in backend > System > Permissions > Orphaned Resources.

Related Pull Requests

See PR #2339 that log the orphaned resources.
See PR #3642 that revert #2339 to not cluttered exception.log.

Fixed Issues (if relevant)

  1. Fixes A lot of exceptions after login in backend  #3625

Screenshots [edited]

At backend login:
image
Grid to list and delete:
image

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Admin Relates to Mage_Admin translations Relates to app/locale PHPStorm labels Nov 12, 2023
@sreichel
Copy link
Contributor

sreichel commented Nov 12, 2023

Nice. Just did a visual review ... whats about disabled extensions?

(thats why ive not completed commented code parts)

Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add strict types for new classes?

kiatng and others added 5 commits November 12, 2023 14:14
Co-authored-by: Sven Reichel <github-sr@hotmail.com>
Co-authored-by: Sven Reichel <github-sr@hotmail.com>
Co-authored-by: Sven Reichel <github-sr@hotmail.com>
…e/Grid.php

Co-authored-by: Sven Reichel <github-sr@hotmail.com>
…e.php

Co-authored-by: Sven Reichel <github-sr@hotmail.com>
@fballiano
Copy link
Contributor

I think a n98-magerun action or a shell script would be enough because it's a one time maintainance thing, but good job anyway!

@luigifab
Copy link
Contributor

luigifab commented Nov 12, 2023

@sreichel do you think that there are a lot of work with find app/ shell/ lib/Mage/ lib/Magento/ lib/Varien/ -name "*.ph*" -type f -exec sed -i '1s/<?php/<?php declare(strict_types=1);/' {} + ? (chatgpt powered)

💣

Edit: .... ~10 files to edit for backend! But, I didn't tested all pages.
loool: var_dump(Mage::app()->getStore()->getId()); => string(1) "0"

@empiricompany
Copy link
Contributor

Good job, but it would appear in all my clients' dashboards who are not developers and wouldn't know what to do.
I agree with @fballiano that for these developer-related tasks, it is better to provide a utility in a shell script or n98-magerun.

Question: I haven't tested it yet, but what happens if I remove the permissions of a temporarily disabled extension? Will they be automatically recreated or will break the extension ?

Unfortunately, there is no system for uninstalling extensions. There are other things to consider, such as cleaning up the core_resource and core_config tables, and rolling back any database changes.
IMHO, I still believe it is better to remove the exception. Otherwise, every time I remove/disable an extension, the problem reoccurs.

@fballiano
Copy link
Contributor

just some quick last minute formal fixes and I'll absolutely approve this

kiatng and others added 5 commits February 8, 2024 19:52
…esourceController.php

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
…e/Grid.php

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
…e.php

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Delete unnecessary docblock
@addison74
Copy link
Contributor

All the other links in the menu are limited to one word. If we rename the new link just "Resources", is it a bad idea? In my opinion no, because it covers everything related to resources.

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda prefer more verbose names expecially if they're more understandable :-)

@fballiano
Copy link
Contributor

another review and we're good to merge

@fballiano
Copy link
Contributor

it would be so nice to have enough reviews for a new feature..

Copy link
Contributor

@addison74 addison74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works as described. Good job @kiatng.

@fballiano fballiano merged commit 1a842ec into OpenMage:main Feb 16, 2024
17 checks passed
@kiatng kiatng deleted the 3625_orphaned_resources branch February 16, 2024 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml PHPStorm translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A lot of exceptions after login in backend
7 participants