Permissions - Code Metrics - Inspection of "Refactoring" - evolution-cms/evolution - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — develop (#716)

by Agel_Nash

created 2018-06-07 21:20 UTC

Permissions A

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	80
Duplicated Lines	5 %

Coupling/Cohesion

Components	1
Dependencies	2

Importance

Changes

Metric	Value
dl	4
loc	80
rs	10
c	0
b	0
f	0
wmc	15
lcom	1
cbo	2

1 Method

Rating	Name	Duplication	Size	Complexity
C	checkPermissions()	4	57	15

How to fix Duplicated Code

<?php namespace EvolutionCMS\Legacy;

/**
 * @class: udperms
 */
class Permissions
{
    /**
     * @var int
     */
    public $user;
    /**
     * @var int
     */
    public $document;
    /**
     * @var int
     */
    public $role;
    /**
     * @var bool
     */
    public $duplicateDoc = false;

    /**
     * @return bool
     */
    public function checkPermissions()
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
    {

        global $udperms_allowroot;
function myFunction($a, $b) {
    // Do something
}
        $modx = evolutionCMS();

        $tblsc = $modx->getFullTableName('site_content');
        $tbldg = $modx->getFullTableName('document_groups');
        $tbldgn = $modx->getFullTableName('documentgroup_names');

        $document = $this->document;
        $role = $this->role;

        if ($role == 1) {
            return true;  // administrator - grant all document permissions
        }

        if ($modx->config['use_udperms'] == 0 || $modx->config['use_udperms'] == "" || !isset($modx->config['use_udperms'])) {
            return true; // permissions aren't in use
        }

        $parent = $modx->db->getValue($modx->db->select('parent', $tblsc, "id='{$this->document}'"));
        if ($document == 0 && $parent == null && $udperms_allowroot == 1) {
            return true;
        } // User is allowed to create new document in root
        if (($this->duplicateDoc == true || $document == 0) && $parent == 0 && $udperms_allowroot == 0) {

            return false; // deny duplicate || create new document at root if Allow Root is No
        }

        // get document groups for current user
        $docgrp = empty($_SESSION['mgrDocgroups']) ? '' : implode(' || dg.document_group = ',
            $_SESSION['mgrDocgroups']);

        /* Note:
            A document is flagged as private whenever the document group that it
            belongs to is assigned or links to a user group. In other words if
            the document is assigned to a document group that is not yet linked
            to a user group then that document will be made public. Documents that
            are private to the manager users will not be private to web users if the
            document group is not assigned to a web user group and visa versa.
         */
        $permissionsok = false;  // set permissions to false

        $rs = $modx->db->select(

            'count(DISTINCT sc.id)',
            "{$tblsc} AS sc 
				LEFT JOIN {$tbldg} AS dg on dg.document = sc.id 
				LEFT JOIN {$tbldgn} dgn ON dgn.id = dg.document_group",
            "sc.id='{$this->document}' AND (" . (empty($docgrp) ? '' : "dg.document_group = " . $docgrp . " ||") . " sc.privatemgr = 0)"
        );
        $limit = $modx->db->getValue($rs);
        if ($limit == 1) {
            $permissionsok = true;
        }

        return $permissionsok;
    }
}


Pull Request — develop (#716)

Permissions A

Complexity

Size/Duplication

Coupling/Cohesion

Importance

1 Method

How to fix Duplicated Code

Duplicated Code

1. Pass all data via parameters

2. Create a class that maintains your state

1		<?php namespace EvolutionCMS\Legacy;
2
3		/**
4		* @class: udperms
5		*/
6		class Permissions
7		{
8		/**
9		* @var int
10		*/
11		public $user;
12		/**
13		* @var int
14		*/
15		public $document;
16		/**
17		* @var int
18		*/
19		public $role;
20		/**
21		* @var bool
22		*/
23		public $duplicateDoc = false;
24
25		/**
26		* @return bool
27		*/
28		public function checkPermissions()
		0 ignored issues – show Coding Style introduced 2018-02-25 17:05 UTC by Report Bug Copy Issue Report `checkPermissions` uses the super-global variable `$_SESSION` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
29		{
30
31		global $udperms_allowroot;
		0 ignored issues – show Compatibility Best Practice introduced 2018-02-21 15:53 UTC by Report Bug Copy Issue Report Use of `global` functionality is not recommended; it makes your code harder to test, and less reusable. Instead of relying on `global` state, we recommend one of these alternatives: 1. Pass all data via parameters function myFunction($a, $b) { // Do something } 2. Create a class that maintains your state class MyClass { private $a; private $b; public function __construct($a, $b) { $this->a = $a; $this->b = $b; } public function myFunction() { // Do something } } Loading history...
32		$modx = evolutionCMS();
33
34		$tblsc = $modx->getFullTableName('site_content');
35		$tbldg = $modx->getFullTableName('document_groups');
36		$tbldgn = $modx->getFullTableName('documentgroup_names');
37
38		$document = $this->document;
39		$role = $this->role;
40
41		if ($role == 1) {
42		return true; // administrator - grant all document permissions
43		}
44
45		if ($modx->config['use_udperms'] == 0 \|\| $modx->config['use_udperms'] == "" \|\| !isset($modx->config['use_udperms'])) {
46		return true; // permissions aren't in use
47		}
48
49		$parent = $modx->db->getValue($modx->db->select('parent', $tblsc, "id='{$this->document}'"));
50	View Code Duplication	if ($document == 0 && $parent == null && $udperms_allowroot == 1) {
51		return true;
52		} // User is allowed to create new document in root
53	View Code Duplication	if (($this->duplicateDoc == true \|\| $document == 0) && $parent == 0 && $udperms_allowroot == 0) {
		0 ignored issues – show Coding Style Best Practice introduced 2018-02-21 15:53 UTC by Report Bug Copy Issue Report It seems like you are loosely comparing two booleans. Considering using the strict comparison `===` instead. When comparing two booleans, it is generally considered safer to use the strict comparison operator. Loading history...
54		return false; // deny duplicate \|\| create new document at root if Allow Root is No
55		}
56
57		// get document groups for current user
58		$docgrp = empty($_SESSION['mgrDocgroups']) ? '' : implode(' \|\| dg.document_group = ',
59		$_SESSION['mgrDocgroups']);
60
61		/* Note:
62		A document is flagged as private whenever the document group that it
63		belongs to is assigned or links to a user group. In other words if
64		the document is assigned to a document group that is not yet linked
65		to a user group then that document will be made public. Documents that
66		are private to the manager users will not be private to web users if the
67		document group is not assigned to a web user group and visa versa.
68		*/
69		$permissionsok = false; // set permissions to false
70
71		$rs = $modx->db->select(
		0 ignored issues – show Comprehensibility introduced 2018-02-21 15:53 UTC by Report Bug Copy Issue Report Avoid variables with short names like $rs. Configured minimum length is 3. Short variable names may make your code harder to understand. Variable names should be self-descriptive. This check looks for variable names who are shorter than a configured minimum. Loading history...
72		'count(DISTINCT sc.id)',
73		"{$tblsc} AS sc
74		LEFT JOIN {$tbldg} AS dg on dg.document = sc.id
75		LEFT JOIN {$tbldgn} dgn ON dgn.id = dg.document_group",
76		"sc.id='{$this->document}' AND (" . (empty($docgrp) ? '' : "dg.document_group = " . $docgrp . " \|\|") . " sc.privatemgr = 0)"
77		);
78		$limit = $modx->db->getValue($rs);
79		if ($limit == 1) {
80		$permissionsok = true;
81		}
82
83		return $permissionsok;
84		}
85		}
86

evolution-cms / evolution

Pull Request — develop (#716)

Permissions A

Complexity

Size/Duplication

Coupling/Cohesion

Importance

1 Method

How to fix Duplicated Code

Duplicated Code

1. Pass all data via parameters

2. Create a class that maintains your state

Duplication Side-by-Side

Filter issues like