LogHandler::initAndWriteLog() - 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:59 UTC

LogHandler::initAndWriteLog() F

↳ Parent: LogHandler

Complexity

Conditions	10
Paths	512

Size

Total Lines	30
Code Lines	20

Duplication

Lines	0
Ratio	0 %

Importance

Changes

Metric	Value
cc	10
eloc	20
nc	512
nop	6
dl	0
loc	30
rs	3.2187
c	0
b	0
f	0

How to fix Complexity

<?php namespace EvolutionCMS\Legacy;

/**
 * logger class.
 *
 * Usage:
 *
 * include_once "log.class.inc.php"; // include_once the class
 * $log = new logHandler;  // create the object
 * $log->initAndWriteLog($msg); // write $msg to log, and populate all other fields as best as possible
 * $log->initAndWriteLog($msg, $internalKey, $username, $action, $id, $itemname); // write $msg and other data to log
 */
class LogHandler
{
    /**
     * Single variable for a log entry
     *
     * @var array
     */
    public $entry = array();

    /**
     * @param string $msg
     * @param string $internalKey
     * @param string $username
     * @param string $action
     * @param string $itemid
     * @param string $itemname
     *
     * @return void
     */
    public function initAndWriteLog(
// 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);
    }
}
        $msg = "",
        $internalKey = "",
        $username = "",
        $action = "",
        $itemid = "",
        $itemname = ""
    ) {
        $modx = evolutionCMS();
        $this->entry['msg'] = $msg; // writes testmessage to the object
        $this->entry['action'] = empty($action) ? $modx->manager->action : $action;    // writes the action to the object

        // User Credentials
        $this->entry['internalKey'] = $internalKey == "" ? $modx->getLoginUserID() : $internalKey;
        $this->entry['username'] = $username == "" ? $modx->getLoginUserName() : $username;

        $this->entry['itemId'] = (empty($itemid) && isset($_REQUEST['id'])) ? (int)$_REQUEST['id'] : $itemid;  // writes the id to the object
        if ($this->entry['itemId'] == 0) {
            $this->entry['itemId'] = "-";
        } // to stop items having id 0

        $this->entry['itemName'] = ($itemname == "" && isset($_SESSION['itemname'])) ? $_SESSION['itemname'] : $itemname; // writes the id to the object
        if ($this->entry['itemName'] == "") {
            $this->entry['itemName'] = "-";
        } // to stop item name being empty

        $this->writeToLog();

        return;
    }

    /**
     * function to write to the log collects all required info, and writes it to the logging table
     *
     * @return void
     */
    public function writeToLog()
// 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);
    }
}
    {
        $modx = evolutionCMS();
        $tbl_manager_log = $modx->getFullTableName('manager_log');

        if ($this->entry['internalKey'] == "") {
            $modx->webAlertAndQuit("Logging error: internalKey not set.");
        }
        if (empty($this->entry['action'])) {
            $modx->webAlertAndQuit("Logging error: action not set.");
        }
        if ($this->entry['msg'] == "") {
            include_once "actionlist.inc.php";
            $this->entry['msg'] = getAction($this->entry['action'], $this->entry['itemId']);
            if ($this->entry['msg'] == "") {
                $modx->webAlertAndQuit("Logging error: couldn't find message to write to log.");
            }
        }

        $fields['timestamp'] = time();
foreach ($collection as $item) {
    $myArray['foo'] = $item->getFoo();

    if ($item->hasBar()) {
        $myArray['bar'] = $item->getBar();
    }

    // do something with $myArray
}
        $fields['internalKey'] = $modx->db->escape($this->entry['internalKey']);
        $fields['username'] = $modx->db->escape($this->entry['username']);
        $fields['action'] = $this->entry['action'];
        $fields['itemid'] = $this->entry['itemId'];
        $fields['itemname'] = $modx->db->escape($this->entry['itemName']);
        $fields['message'] = $modx->db->escape($this->entry['msg']);
        $fields['ip'] = $this->getUserIP();
        $fields['useragent'] = $_SERVER['HTTP_USER_AGENT'];

        $insert_id = $modx->db->insert($fields, $tbl_manager_log);
        if (!$insert_id) {
0   == false // true
0   == null  // true
123 == false // false
123 == null  // false

// It is often better to use strict comparison
0 === false // false
0 === null  // false
            $modx->messageQuit("Logging error: couldn't save log to table! Error code: " . $modx->db->getLastError());
        } else {
            $limit = (isset($modx->config['manager_log_limit'])) ? (int)$modx->config['manager_log_limit'] : 3000;
            $trim = (isset($modx->config['manager_log_trim'])) ? (int)$modx->config['manager_log_trim'] : 100;
            if (($insert_id % $trim) === 0) {
                $modx->rotate_log('manager_log', $limit, $trim);
            }
        }
    }

    private function getUserIP() {
// 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);
    }
}
        if( array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER) && !empty($_SERVER['HTTP_X_FORWARDED_FOR']) ) {
            if (strpos($_SERVER['HTTP_X_FORWARDED_FOR'], ',')>0) {
                $addr = explode(",",$_SERVER['HTTP_X_FORWARDED_FOR']);
                return trim($addr[0]);
            } else {
                return $_SERVER['HTTP_X_FORWARDED_FOR'];
            }
        }
        else {
            return $_SERVER['REMOTE_ADDR'];
        }
    }
}


1			<?php namespace EvolutionCMS\Legacy;
2
3			/**
4			* logger class.
5			*
6			* Usage:
7			*
8			* include_once "log.class.inc.php"; // include_once the class
9			* $log = new logHandler; // create the object
10			* $log->initAndWriteLog($msg); // write $msg to log, and populate all other fields as best as possible
11			* $log->initAndWriteLog($msg, $internalKey, $username, $action, $id, $itemname); // write $msg and other data to log
12			*/
13			class LogHandler
14			{
15			/**
16			* Single variable for a log entry
17			*
18			* @var array
19			*/
20			public $entry = array();
21
22			/**
23			* @param string $msg
24			* @param string $internalKey
25			* @param string $username
26			* @param string $action
27			* @param string $itemid
28			* @param string $itemname
29			*
30			* @return void
31			*/
32			public function initAndWriteLog(
			0 ignored issues – show Coding Style introduced 2018-02-25 17:05 UTC by Report Bug Copy Issue Report `initAndWriteLog` uses the super-global variable `$_REQUEST` 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... Coding Style introduced 2018-02-25 17:05 UTC by Report Bug Copy Issue Report `initAndWriteLog` 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...
33			$msg = "",
34			$internalKey = "",
35			$username = "",
36			$action = "",
37			$itemid = "",
38			$itemname = ""
39			) {
40			$modx = evolutionCMS();
41			$this->entry['msg'] = $msg; // writes testmessage to the object
42			$this->entry['action'] = empty($action) ? $modx->manager->action : $action; // writes the action to the object
43
44			// User Credentials
45			$this->entry['internalKey'] = $internalKey == "" ? $modx->getLoginUserID() : $internalKey;
46			$this->entry['username'] = $username == "" ? $modx->getLoginUserName() : $username;
47
48			$this->entry['itemId'] = (empty($itemid) && isset($_REQUEST['id'])) ? (int)$_REQUEST['id'] : $itemid; // writes the id to the object
49			if ($this->entry['itemId'] == 0) {
50			$this->entry['itemId'] = "-";
51			} // to stop items having id 0
52
53			$this->entry['itemName'] = ($itemname == "" && isset($_SESSION['itemname'])) ? $_SESSION['itemname'] : $itemname; // writes the id to the object
54			if ($this->entry['itemName'] == "") {
55			$this->entry['itemName'] = "-";
56			} // to stop item name being empty
57
58			$this->writeToLog();
59
60			return;
61			}
62
63			/**
64			* function to write to the log collects all required info, and writes it to the logging table
65			*
66			* @return void
67			*/
68			public function writeToLog()
			0 ignored issues – show Coding Style introduced 2018-05-17 22:20 UTC by Report Bug Copy Issue Report `writeToLog` uses the super-global variable `$_SERVER` 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...
69			{
70			$modx = evolutionCMS();
71			$tbl_manager_log = $modx->getFullTableName('manager_log');
72
73			if ($this->entry['internalKey'] == "") {
74			$modx->webAlertAndQuit("Logging error: internalKey not set.");
75			}
76			if (empty($this->entry['action'])) {
77			$modx->webAlertAndQuit("Logging error: action not set.");
78			}
79			if ($this->entry['msg'] == "") {
80			include_once "actionlist.inc.php";
81			$this->entry['msg'] = getAction($this->entry['action'], $this->entry['itemId']);
82			if ($this->entry['msg'] == "") {
83			$modx->webAlertAndQuit("Logging error: couldn't find message to write to log.");
84			}
85			}
86
87			$fields['timestamp'] = time();
			0 ignored issues – show Coding Style Comprehensibility introduced 2018-02-21 15:53 UTC by Report Bug Copy Issue Report `$fields` was never initialized. Although not strictly required by PHP, it is generally a good practice to add `$fields = array();` before regardless. Adding an explicit array definition is generally preferable to implicit array definition as it guarantees a stable state of the code. Let’s take a look at an example: foreach ($collection as $item) { $myArray['foo'] = $item->getFoo(); if ($item->hasBar()) { $myArray['bar'] = $item->getBar(); } // do something with $myArray } As you can see in this example, the array `$myArray` is initialized the first time when the foreach loop is entered. You can also see that the value of the `bar` key is only written conditionally; thus, its value might result from a previous iteration. This might or might not be intended. To make your intention clear, your code more readible and to avoid accidental bugs, we recommend to add an explicit initialization `$myArray = array()` either outside or inside the foreach loop. Loading history...
88			$fields['internalKey'] = $modx->db->escape($this->entry['internalKey']);
89			$fields['username'] = $modx->db->escape($this->entry['username']);
90			$fields['action'] = $this->entry['action'];
91			$fields['itemid'] = $this->entry['itemId'];
92			$fields['itemname'] = $modx->db->escape($this->entry['itemName']);
93			$fields['message'] = $modx->db->escape($this->entry['msg']);
94			$fields['ip'] = $this->getUserIP();
95			$fields['useragent'] = $_SERVER['HTTP_USER_AGENT'];
96
97			$insert_id = $modx->db->insert($fields, $tbl_manager_log);
98			if (!$insert_id) {
			0 ignored issues – show Bug Best Practice introduced 2018-02-26 22:21 UTC by Report Bug Copy Issue Report The expression `$insert_id` of type `null\|integer` is loosely compared to `false`; this is ambiguous if the integer can be zero. You might want to explicitly use `=== null` instead. In PHP, under loose comparison (like `==`, or `!=`, or `switch` conditions), values of different types might be equal. For `integer` values, zero is a special case, in particular the following results might be unexpected: 0 == false // true 0 == null // true 123 == false // false 123 == null // false // It is often better to use strict comparison 0 === false // false 0 === null // false Loading history...
99			$modx->messageQuit("Logging error: couldn't save log to table! Error code: " . $modx->db->getLastError());
100			} else {
101			$limit = (isset($modx->config['manager_log_limit'])) ? (int)$modx->config['manager_log_limit'] : 3000;
102			$trim = (isset($modx->config['manager_log_trim'])) ? (int)$modx->config['manager_log_trim'] : 100;
103			if (($insert_id % $trim) === 0) {
104			$modx->rotate_log('manager_log', $limit, $trim);
105			}
106			}
107			}
108
109			private function getUserIP() {
			0 ignored issues – show Documentation introduced 2018-03-25 21:45 UTC by Report Bug Copy Issue Report The return type could not be reliably inferred; please add a `@return` annotation. Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a `@return` annotation as described here. Loading history... Coding Style introduced 2018-03-25 21:45 UTC by Report Bug Copy Issue Report `getUserIP` uses the super-global variable `$_SERVER` 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...
110			if( array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER) && !empty($_SERVER['HTTP_X_FORWARDED_FOR']) ) {
111			if (strpos($_SERVER['HTTP_X_FORWARDED_FOR'], ',')>0) {
112			$addr = explode(",",$_SERVER['HTTP_X_FORWARDED_FOR']);
113			return trim($addr[0]);
114			} else {
115			return $_SERVER['HTTP_X_FORWARDED_FOR'];
116			}
117			}
118			else {
119			return $_SERVER['REMOTE_ADDR'];
120			}
121			}
122			}
123

evolution-cms / evolution

Pull Request — develop (#716)

LogHandler::initAndWriteLog() F

Complexity

Size

Duplication

Importance

How to fix Complexity

Long Method

Duplication Side-by-Side

Filter issues like