e107inc / e107

e107 Bootstrap CMS (Content Management System) v2 with PHP, MySQL, HTML5, jQuery and Twitter Bootstrap. Issue Discussion Room: https://gitter.im/e107inc/e107
https://e107.org
GNU General Public License v3.0
318 stars 212 forks source link

Unify logic of `e_user_model::checkAdminPerms()` and `getperms()` #5070

Closed Deltik closed 9 months ago

Deltik commented 9 months ago

Motivation and Context

I had some concerns about https://github.com/e107inc/e107/commit/44526b435c85620d0fb4cff6858f3ef7fc615ca6 and https://github.com/e107inc/e107/commit/001799cb5f95b33d9c227acb41d5d657e21b7d88:

The modified method signature of e_user_model::checkAdminPerms() defeats the encapsulation offered by moving the global getperms() to e_user_model::checkAdminPerms() per user because $ap would override the object's internal state for the purposes of simulating permissions.

To remedy this, I suggest that we extract getperms() into a third static method that doesn't depend on state―perhaps called e_userperms::checkAdminPerms()―and have both getperms() and e_user_model::checkAdminPerms() call it to check the admin permissions. This way, we can have both backwards compatibility and encapsulation for the two different styles we support.


I have another concern, which is the extra logic for plugins. I would move this into a new method called e_user_model::checkPluginAdminPerms($plugin_name) which then calls e_user_model::checkAdminPerms() once it has figured out the $perm_str.

Description

Along with extensive documentation, getperms() is now deprecated and its replacements now have first-class support:


Partially reverts: https://github.com/e107inc/e107/commit/44526b43

Reverts: https://github.com/e107inc/e107/commit/001799cb

Fixes: https://github.com/e107inc/e107/issues/5064

How Has This Been Tested?

As this is a refactoring only, all existing tests of getperms() pass.

I also added some tests for the misuse cases of whitespace in the second argument or an integer in the first argument of getperms(0, ' ').

Types of Changes

Checklist

codeclimate[bot] commented 9 months ago

Code Climate has analyzed commit dd36fbd5 and detected 183 issues on this pull request.

Here's the issue category breakdown:

Category Count
Bug Risk 183

The test coverage on the diff in this pull request is 77.5% (80% is the threshold).

This pull request will bring the total coverage in the repository to 34.4% (-0.2% change).

View more on Code Climate.

CaMer0n commented 9 months ago

Thank you!! 🥇 👍