Add rector/rector checks to improve code quality
#20114
Conversation
src/Group_User.php
Outdated
| foreach ($parents_members as $parent) { | ||
| $members[] = $parent; | ||
| } | ||
| $members = $parents_members; |
There was a problem hiding this comment.
$members = self::getParentsMembers($item, '');
src/Stat.php
Outdated
| @@ -435,7 +433,7 @@ public static function showTable($itemtype, $type, $date1, $date2, $start, array | |||
| // Set display type for export if define | |||
| $output_type = $_GET["display_type"] ?? Search::HTML_OUTPUT; | |||
| $output = SearchEngine::getOutputForLegacyKey($output_type); | |||
| $is_html_output = is_a($output, HTMLSearchOutput::class); | |||
| $is_html_output = $output instanceof \Glpi\Search\Output\HTMLSearchOutput; | |||
There was a problem hiding this comment.
I think the command executed by @cedric-anne did the change automatically.
Maybe it is considered a good practice because it makes the code more readable in some cases ?
Indeed sometimes the class name is not unique and the developers will need to check the full namespace to be sure of what is being used.
There was a problem hiding this comment.
It just uses the FQCN because the ->withImportNames() config line is commented. I will fix it manually.
src/Stat.php
Outdated
| @@ -727,9 +725,9 @@ public static function showTable($itemtype, $type, $date1, $date2, $start, array | |||
|
|
|||
| $timedisplay = $nb_solved > 0 ? array_sum($data) / $nb_solved : 0; | |||
|
|
|||
| if ($is_html_output || is_a($output, Pdf::class)) { | |||
| if ($is_html_output || $output instanceof \Glpi\Search\Output\Pdf) { | |||
src/Stat.php
Outdated
| $timedisplay = Html::timestampToString($timedisplay, 0, false); | ||
| } elseif (is_a($output, Csv::class)) { | ||
| } elseif ($output instanceof \Glpi\Search\Output\Csv) { |
There was a problem hiding this comment.
Still present in use, no need for full name.
|
That's a must have. Fine to have rector. I just wonder if it's the right time to integrate it. |
Seems good for met: there not so much changes, and impact is low. |
src/Glpi/OAuth/Server.php
Outdated
| @@ -257,14 +257,14 @@ private static function doGenerateKeys(): void | |||
| try { | |||
| $key = openssl_pkey_new($config); | |||
| } catch (OpensslException $e) { | |||
| throw new RuntimeException("Unable to generate keys: " . $e->getMessage(), $e->getCode(), $e); | |||
| throw new RuntimeException("Unable to generate keys: " . $e->getMessage(), 0, previous: $e); | |||
There was a problem hiding this comment.
Small note but is there a reason to manually set the code to 0 here ? Since you use named parameters you could avoid setting the code if you don't need to.
There was a problem hiding this comment.
Rector will says that the code should be set from the previous exception if it is not explicitely set. For me, it does not make sense.
We could remove this specific rule if we consider it too annoying.
There was a problem hiding this comment.
It make sense to set it from the previous exception, why shouldn't we ?
There was a problem hiding this comment.
I changed the latest commit to keep the previous exception code. It does not change much, but I had to change some tests.
Checklist before requesting a review
Description
Rector is a powerfull tool that is able to fasten refactoring by automatically patching the code. It has many rules (700+) for many different cases.
For the moment, I applied only some of the "Code Quality" rule set (https://getrector.com/find-rule?rectorSet=core-code-quality&activeRectorSetGroup=core). I tried to apply only rules that result in a small diff, to ease the validation of the PR.
Later, this tool may be usefull to:
We may also, in the future, provide rector rules sets to ease the migration from a GLPI version to another, in an extension dedicated to GLPI plugins.
This PR should be consider as an initial step. Once it will be validated and merged, everyone will be able to quickly add additional rules.