Orphaned ACL resource exceptions are now logged only in developer mode

[empiricompany]

Description (*)

more details here:
#3625

Revert
https://github.com/OpenMage/magento-lts/pull/2339/files#diff-1048571d4baf38d246708d5953af464948b5591c2c8d648036394d6101ba8022

Related Pull Requests

Fixed Issues (if relevant)

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

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[sreichel]

This code is not complete.
Its not that easy to solve for disabled modules.

Better delete orphan records (and maybe add log otation, or patch it away?)

I see more pros then cons with logging.

[empiricompany]

Sorry @sreichel this is not intended to resolve the orphan data but only to not log an exception every time i log in in the backend until we have a solution

[Flyingmana]

I recommend to apply this change temporarily via the composer patcher if this causes you problems.
Iam against merging it, as logging this inconsistency is a good method the get aware of it.
And yes we should have a way to clean these up, maybe a script is ok?

The original code also had a (commented out) part in, which automatically cleans it up. Iam not a fan of this, but making it configurable would also be an option.

Would one of these options be acceptable?

[fballiano]

I agree with @Flyingmana that logging is a good way to find and cleanup problems, and I agree with the sentiment of the original PR thus I'd leave it as it is, cleanup should be quite easy and there's #3647 on the way

[empiricompany]

what if we wrap the exception in this if statement?

if (Mage::getIsDeveloperMode()) {
   Mage::logException($e);
}

[Flyingmana]

what if we wrap the exception in this if statement?

if (Mage::getIsDeveloperMode()) {
   Mage::logException($e);
}

acceptable for me

[sreichel]

what if we wrap the exception in this if statement?

if (Mage::getIsDeveloperMode()) {
   Mage::logException($e);
}

Perfect for me. It does not fill logs in production, but pops up in local env ... @Sdfendor

[Sdfendor]

I agree, it's acceptable.
The necessity for a cleanup/deletion mechanism for orphaned entries meanwhile stays the same. But this of course doesn't need to happen in this PR.

[m-overlund]

Perfect!

[Flyingmana]

can please someone provide a better title for the PR based on the new changes?

[empiricompany]

can please someone provide a better title for the PR based on the new changes?

done