From 49a40ea1262edb404cd7c5daabe729129104c0d3 Mon Sep 17 00:00:00 2001 From: Aleksander Machniak Date: Sat, 23 Mar 2024 13:48:51 +0100 Subject: [PATCH] Code improvements --- phpstan-baseline.neon | 150 ------------------ .../example_addressbook_backend.php | 4 +- plugins/identicon/tests/Identicon.php | 2 +- plugins/jqueryui/jqueryui.php | 1 - .../managesieve/lib/Roundcube/rcube_sieve.php | 10 +- .../lib/Roundcube/rcube_sieve_engine.php | 14 +- .../lib/Roundcube/rcube_sieve_script.php | 17 +- .../lib/Roundcube/rcube_sieve_vacation.php | 32 ++-- plugins/managesieve/managesieve.php | 4 +- .../squirrelmail_usercopy.php | 15 +- .../vcard_attachments/vcard_attachments.php | 7 +- 11 files changed, 60 insertions(+), 196 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 20b90cde81c..3ff7240b158 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1,145 +1,5 @@ parameters: ignoreErrors: - - - message: "#^Method example_addressbook_backend\\:\\:add_to_group\\(\\) should return int but returns false\\.$#" - count: 1 - path: plugins/example_addressbook/example_addressbook_backend.php - - - - message: "#^Method example_addressbook_backend\\:\\:remove_from_group\\(\\) should return int but returns false\\.$#" - count: 1 - path: plugins/example_addressbook/example_addressbook_backend.php - - - - message: "#^Method Identicon_Plugin\\:\\:setUpBeforeCLass\\(\\) does not match parent method name\\: PHPUnit\\\\Framework\\\\TestCase\\:\\:setUpBeforeClass\\(\\)\\.$#" - count: 1 - path: plugins/identicon/tests/Identicon.php - - - - message: "#^Foreach overwrites \\$skin with its value variable\\.$#" - count: 1 - path: plugins/jqueryui/jqueryui.php - - - - message: "#^Method rcube_sieve\\:\\:__construct\\(\\) with return type void returns mixed but should not return anything\\.$#" - count: 2 - path: plugins/managesieve/lib/Roundcube/rcube_sieve.php - - - - message: "#^Access to an undefined property rcube_sieve_engine\\:\\:\\$master_file\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php - - - - message: "#^Call to function is_array\\(\\) with array will always evaluate to true\\.$#" - count: 2 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php - - - - message: "#^Comparison operation \"\\>\" between int\\<1, max\\> and 0 is always true\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php - - - - message: "#^Foreach overwrites \\$rid with its key variable\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php - - - - message: "#^Result of && is always false\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php - - - - message: "#^Strict comparison using \\=\\=\\= between string and null will always evaluate to false\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php - - - - message: "#^Call to function is_array\\(\\) with array will always evaluate to true\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Call to function is_array\\(\\) with string will always evaluate to false\\.$#" - count: 2 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Implicit array creation is not allowed \\- variable \\$tests might not exist\\.$#" - count: 9 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Loose comparison using \\=\\= between 'INGO' and 'INGO' will always evaluate to true\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Method rcube_sieve_script\\:\\:_tokenize_rule\\(\\) should return array but returns null\\.$#" - count: 2 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Method rcube_sieve_script\\:\\:update_rule\\(\\) should return bool but returns int\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Result of && is always false\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Unreachable statement \\- code above always terminates\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Variable \\$i might not be defined\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Variable \\$pos might not be defined\\.$#" - count: 3 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_script.php - - - - message: "#^Call to function is_array\\(\\) with array will always evaluate to true\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php - - - - message: "#^Implicit array creation is not allowed \\- variable \\$date_value does not exist\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php - - - - message: "#^Implicit array creation is not allowed \\- variable \\$vacation does not exist\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php - - - - message: "#^Implicit array creation is not allowed \\- variable \\$vacation_action does not exist\\.$#" - count: 1 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php - - - - message: "#^Variable variables are not allowed\\.$#" - count: 3 - path: plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php - - - - message: "#^Access to an undefined property rcube\\:\\:\\$action\\.$#" - count: 1 - path: plugins/managesieve/managesieve.php - - - - message: "#^Access to an undefined property rcube\\:\\:\\$task\\.$#" - count: 1 - path: plugins/managesieve/managesieve.php - - message: "#^Method HTTPSocket\\:\\:fetch_header\\(\\) should return array but returns string\\.$#" count: 1 @@ -240,16 +100,6 @@ parameters: count: 1 path: plugins/password/password.php - - - message: "#^Implicit array creation is not allowed \\- variable \\$rec might not exist\\.$#" - count: 1 - path: plugins/squirrelmail_usercopy/squirrelmail_usercopy.php - - - - message: "#^Right side of && is always true\\.$#" - count: 1 - path: plugins/vcard_attachments/vcard_attachments.php - - message: "#^Foreach overwrites \\$cid with its value variable\\.$#" count: 1 diff --git a/plugins/example_addressbook/example_addressbook_backend.php b/plugins/example_addressbook/example_addressbook_backend.php index 42950869123..d97caf7abd0 100644 --- a/plugins/example_addressbook/example_addressbook_backend.php +++ b/plugins/example_addressbook/example_addressbook_backend.php @@ -240,11 +240,11 @@ public function rename_group($gid, $newname, &$newid) public function add_to_group($group_id, $ids) { - return false; + return 0; } public function remove_from_group($group_id, $ids) { - return false; + return 0; } } diff --git a/plugins/identicon/tests/Identicon.php b/plugins/identicon/tests/Identicon.php index 1852c170fc6..80f42b914ea 100644 --- a/plugins/identicon/tests/Identicon.php +++ b/plugins/identicon/tests/Identicon.php @@ -4,7 +4,7 @@ class Identicon_Plugin extends TestCase { - public static function setUpBeforeCLass(): void + public static function setUpBeforeClass(): void { include_once __DIR__ . '/../identicon.php'; } diff --git a/plugins/jqueryui/jqueryui.php b/plugins/jqueryui/jqueryui.php index 860be60a029..77eb0e51487 100644 --- a/plugins/jqueryui/jqueryui.php +++ b/plugins/jqueryui/jqueryui.php @@ -43,7 +43,6 @@ public function init() $this->include_script('js/jquery-ui.min.js'); // include UI stylesheet - $skin = $rcmail->config->get('skin'); $ui_map = $rcmail->config->get('jquery_ui_skin_map', self::$skin_map); $skins = array_keys($rcmail->output->skins); $skins[] = 'elastic'; diff --git a/plugins/managesieve/lib/Roundcube/rcube_sieve.php b/plugins/managesieve/lib/Roundcube/rcube_sieve.php index 49c21030559..eed90818fb7 100644 --- a/plugins/managesieve/lib/Roundcube/rcube_sieve.php +++ b/plugins/managesieve/lib/Roundcube/rcube_sieve.php @@ -81,7 +81,8 @@ public function __construct($username, $password = '', $host = 'localhost', $por $result = $this->sieve->connect($host, $port, $options, $usetls); if (is_a($result, 'PEAR_Error')) { - return $this->_set_error(self::ERROR_CONNECTION); + $this->_set_error(self::ERROR_CONNECTION); + return; } $authz = null; @@ -98,7 +99,8 @@ public function __construct($username, $password = '', $host = 'localhost', $por $result = $this->sieve->login($username, $password, $auth_type ? strtoupper($auth_type) : null, $authz); if (is_a($result, 'PEAR_Error')) { - return $this->_set_error(self::ERROR_LOGIN); + $this->_set_error(self::ERROR_LOGIN); + return; } $this->exts = $this->get_extensions(); @@ -291,6 +293,8 @@ public function remove($name = null) /** * Gets list of supported by server Sieve extensions + * + * @return array|false */ public function get_extensions() { @@ -321,6 +325,8 @@ public function get_extensions() /** * Gets list of scripts from server + * + * @return array|false */ public function get_scripts() { diff --git a/plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php b/plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php index 6a9505cb215..a811a7c43fe 100644 --- a/plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php +++ b/plugins/managesieve/lib/Roundcube/rcube_sieve_engine.php @@ -30,6 +30,7 @@ class rcube_sieve_engine protected $errors; protected $form; protected $list; + protected $master_file; protected $tips = []; protected $script = []; protected $exts = []; @@ -1475,7 +1476,7 @@ public function filter_form($attrib) $_SESSION['managesieve-compact-form'] = $compact; // do not allow creation of new filters - if ($fid === null && in_array('new_filter', $this->disabled_actions)) { + if ($fid === '' && in_array('new_filter', $this->disabled_actions)) { $this->rc->output->show_message('managesieve.disabledaction', 'error'); return; } @@ -2248,7 +2249,7 @@ public function action_div($fid, $id, $div = true) if ($action['type'] == 'redirect') { $parts = explode('@', $action['target']); - if (count($parts) > 0) { + if (count($parts) > 1) { $action['domain'] = array_pop($parts); $action['target'] = implode('@', $parts); } @@ -2823,7 +2824,7 @@ protected function mod_mailbox($mailbox, $mode = 'out') /** * List sieve scripts * - * @return array Scripts list + * @return array|false Scripts list */ public function list_scripts() { @@ -3046,7 +3047,7 @@ public function deactivate_script($name) $name .= $extension; $rid = 0; - foreach ($script as $rid => $rules) { + foreach ($script as $rules) { foreach ($rules['actions'] as $action) { if ($action['type'] == 'include' && empty($action['global']) && $action['target'] == $name @@ -3054,6 +3055,7 @@ public function deactivate_script($name) break 2; } } + $rid++; } // Entry found @@ -3396,7 +3398,7 @@ protected function action_email_input($i, $field) // According to RFC5230 the :from string must specify a valid [RFC2822] mailbox-list // we'll try to extract addresses and validate them separately $from = rcube_mime::decode_address_list($this->form['actions'][$i][$field], null, true, RCUBE_CHARSET); - foreach ((array) $from as $idx => $addr) { + foreach ($from as $idx => $addr) { if (empty($addr['mailto']) || !rcube_utils::check_email($addr['mailto'])) { $this->errors['actions'][$i][$field] = $this->plugin->gettext('noemailwarning'); break; @@ -3406,7 +3408,7 @@ protected function action_email_input($i, $field) } // Only one address is allowed (at least on cyrus imap) - if (is_array($from) && count($from) > 1) { + if (count($from) > 1) { $this->errors['actions'][$i][$field] = $this->plugin->gettext('noemailwarning'); } diff --git a/plugins/managesieve/lib/Roundcube/rcube_sieve_script.php b/plugins/managesieve/lib/Roundcube/rcube_sieve_script.php index 344b6dc9197..b82e24876df 100644 --- a/plugins/managesieve/lib/Roundcube/rcube_sieve_script.php +++ b/plugins/managesieve/lib/Roundcube/rcube_sieve_script.php @@ -123,7 +123,7 @@ public function size() * @param int $index Rule index * @param array $content Rule content (as array) * - * @return bool True on success, False otherwise + * @return int|false Rule index on success, False otherwise */ public function update_rule($index, $content) { @@ -146,7 +146,8 @@ public function update_rule($index, $content) public function set_var($name, $value, $mods = []) { // Check if variable exists - for ($i = 0, $len = count($this->vars); $i < $len; $i++) { + $i = 0; + for ($len = count($this->vars); $i < $len; $i++) { if ($this->vars[$i]['name'] == $name) { break; } @@ -653,6 +654,7 @@ private function _parse_text($script) $this->set_var($matches[1], $matches[2]); } // Horde-Ingo format + // @phpstan-ignore-next-line elseif (!empty($options['format']) && $options['format'] == 'INGO' && preg_match('/^# (.*)/', $line, $matches) ) { @@ -711,7 +713,7 @@ private function _parse_text($script) * @param string $content The whole script content * @param int &$position Start position in the script * - * @return array Rule data + * @return array|null Rule data */ private function _tokenize_rule($content, &$position) { @@ -724,6 +726,7 @@ private function _tokenize_rule($content, &$position) $join = false; $join_not = false; $length = strlen($content); + $tests = []; // disabled rule (false + comment): if false # ..... if (preg_match('/^\s*false\s+#\s*/i', substr($content, $position, 20), $m)) { @@ -906,7 +909,7 @@ private function _tokenize_rule($content, &$position) * @param int &$position Start position in the script * @param string $end End of text separator * - * @return array Array of parsed action type/target pairs + * @return array|null Array of parsed action type/target pairs */ private function _parse_actions($content, &$position, $end = '}') { @@ -1185,7 +1188,7 @@ private function action_arguments(&$tokens, $bool_args, $val_args = []) * Escape special chars into quoted string value or multi-line string * or list of strings * - * @param string $str Text or array (list) of strings + * @param array|string $str Text or array (list) of strings * * @return string Result text */ @@ -1255,7 +1258,8 @@ public static function tokenize($str, $num = 0, &$position = 0) switch ($str[$position]) { // Quoted string case '"': - for ($pos = $position + 1; $pos < $length; $pos++) { + $pos = $position + 1; + for ($pos; $pos < $length; $pos++) { if ($str[$pos] == '"') { break; } @@ -1281,7 +1285,6 @@ public static function tokenize($str, $num = 0, &$position = 0) case ']': $position++; return $result; - break; // list/test separator (<< reindent once https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues/7179 is fixed) case ',': // command separator (<< reindent once https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues/7179 is fixed) diff --git a/plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php b/plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php index 80f198ffda5..63d89ef7e40 100644 --- a/plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php +++ b/plugins/managesieve/lib/Roundcube/rcube_sieve_vacation.php @@ -196,13 +196,15 @@ protected function vacation_post() $target_domain = rcube_utils::get_input_string('action_domain', rcube_utils::INPUT_POST); $interval_type = $interval_type == 'seconds' ? 'seconds' : 'days'; - $vacation_action['type'] = 'vacation'; - $vacation_action['reason'] = $this->strip_value(str_replace("\r\n", "\n", $reason), true); - $vacation_action['subject'] = trim($subject); - $vacation_action['from'] = trim($from); - $vacation_action['addresses'] = $addresses; - $vacation_action[$interval_type] = $interval; $vacation_tests = !empty($this->vacation['tests']) ? $this->vacation['tests'] : []; + $vacation_action = [ + 'type' => 'vacation', + 'reason' => $this->strip_value(str_replace("\r\n", "\n", $reason), true), + 'subject' => trim($subject), + 'from' => trim($from), + 'addresses' => $addresses, + ]; + $vacation_action[$interval_type] = $interval; foreach ((array) $vacation_action['addresses'] as $aidx => $address) { $vacation_action['addresses'][$aidx] = $address = trim($address); @@ -219,7 +221,7 @@ protected function vacation_post() // According to RFC5230 the :from string must specify a valid [RFC2822] mailbox-list // we'll try to extract addresses and validate them separately $from = rcube_mime::decode_address_list($vacation_action['from'], null, true, RCUBE_CHARSET); - foreach ((array) $from as $idx => $addr) { + foreach ($from as $idx => $addr) { if (empty($addr['mailto']) || !rcube_utils::check_email($addr['mailto'])) { $error = $from_error = 'noemailwarning'; break; @@ -229,7 +231,7 @@ protected function vacation_post() } // Only one address is allowed (at least on cyrus imap) - if (is_array($from) && count($from) > 1) { + if (count($from) > 1) { $error = $from_error = 'noemailwarning'; } @@ -438,6 +440,7 @@ public function vacation_form($attrib) $date_format = $this->rc->config->get('date_format', 'Y-m-d'); $time_format = $this->rc->config->get('time_format', 'H:i'); + $date_value = []; if ($date_extension || $regex_extension) { $date_from = new html_inputfield(['name' => 'vacation_datefrom', 'id' => 'vacation_datefrom', 'class' => 'datepicker form-control', 'size' => 12]); @@ -447,7 +450,6 @@ public function vacation_form($attrib) if ($date_extension) { $time_from = new html_inputfield(['name' => 'vacation_timefrom', 'id' => 'vacation_timefrom', 'size' => 7, 'class' => 'form-control']); $time_to = new html_inputfield(['name' => 'vacation_timeto', 'id' => 'vacation_timeto', 'size' => 7, 'class' => 'form-control']); - $date_value = []; if (!empty($this->vacation['tests'])) { foreach ((array) $this->vacation['tests'] as $test) { @@ -751,11 +753,13 @@ public function set_vacation($data) $date_extension = in_array('date', $this->exts); $regex_extension = in_array('regex', $this->exts); - $vacation['type'] = 'vacation'; - $vacation['reason'] = $this->strip_value(str_replace("\r\n", "\n", $data['message']), true); - $vacation['addresses'] = $data['addresses']; - $vacation['subject'] = trim($data['subject']); - $vacation['from'] = trim($data['from']); + $vacation = [ + 'type' => 'vacation', + 'reason' => $this->strip_value(str_replace("\r\n", "\n", $data['message']), true), + 'addresses' => $data['addresses'], + 'subject' => trim($data['subject']), + 'from' => trim($data['from']), + ]; $vacation_tests = (array) $this->vacation['tests']; foreach ((array) $vacation['addresses'] as $aidx => $address) { diff --git a/plugins/managesieve/managesieve.php b/plugins/managesieve/managesieve.php index b6f320a2e4b..28933e87fac 100644 --- a/plugins/managesieve/managesieve.php +++ b/plugins/managesieve/managesieve.php @@ -59,8 +59,8 @@ public function init() $this->register_action('plugin.managesieve-save', [$this, 'managesieve_save']); $this->register_action('plugin.managesieve-saveraw', [$this, 'managesieve_saveraw']); - $task = $this->rc->task ?? null; - $action = $this->rc->action ?? null; + $task = $this->rc->task ?? null; // @phpstan-ignore-line + $action = $this->rc->action ?? null; // @phpstan-ignore-line if ($task == 'settings') { $this->add_hook('settings_actions', [$this, 'settings_actions']); diff --git a/plugins/squirrelmail_usercopy/squirrelmail_usercopy.php b/plugins/squirrelmail_usercopy/squirrelmail_usercopy.php index 55682e5fe7d..b88cba61c3b 100644 --- a/plugins/squirrelmail_usercopy/squirrelmail_usercopy.php +++ b/plugins/squirrelmail_usercopy/squirrelmail_usercopy.php @@ -253,12 +253,15 @@ private function read_squirrel_prefs($uname) . ' WHERE `owner` = ?', $uname); // ? is replaced with emailaddress // parse address book - while ($sql_array = $db->fetch_assoc($sql_result)) { // fetch one row from result - $rec['name'] = rcube_charset::convert(rtrim($sql_array['nickname']), $db_charset); - $rec['firstname'] = rcube_charset::convert(rtrim($sql_array['firstname']), $db_charset); - $rec['surname'] = rcube_charset::convert(rtrim($sql_array['lastname']), $db_charset); - $rec['email'] = rcube_charset::convert(rtrim($sql_array['email']), $db_charset); - $rec['notes'] = rcube_charset::convert(rtrim($sql_array['label']), $db_charset); + // fetch one row from result + while ($sql_array = $db->fetch_assoc($sql_result)) { + $rec = [ + 'name' => rcube_charset::convert(rtrim($sql_array['nickname']), $db_charset), + 'firstname' => rcube_charset::convert(rtrim($sql_array['firstname']), $db_charset), + 'surname' => rcube_charset::convert(rtrim($sql_array['lastname']), $db_charset), + 'email' => rcube_charset::convert(rtrim($sql_array['email']), $db_charset), + 'notes' => rcube_charset::convert(rtrim($sql_array['label']), $db_charset), + ]; if ($rec['name'] && $rec['email']) { $this->abook[] = $rec; diff --git a/plugins/vcard_attachments/vcard_attachments.php b/plugins/vcard_attachments/vcard_attachments.php index aa1f9771d0c..8bbc3cae787 100644 --- a/plugins/vcard_attachments/vcard_attachments.php +++ b/plugins/vcard_attachments/vcard_attachments.php @@ -211,11 +211,8 @@ public function save_vcard() if (!empty($part) && ($part_vcards = rcube_vcard::import($part))) { foreach ($mime_ids as $id) { - if (!empty($part_vcards[$id]) - && ($vcard = $part_vcards[$id]) - && !empty($vcard->email) - && !empty($vcard->email[0]) - ) { + $vcard = $part_vcards[$id] ?? null; + if ($vcard && !empty($vcard->email) && !empty($vcard->email[0])) { $vcards[] = $vcard; } }