Update minimum PHP version to 7.3 and other fixes #30

NicolasCARPi · 2020-04-28T03:37:19Z

Hello,

This is a nice project you have here, congrats! :)

Here is my modest contribution. The changeset is bigger than first intended, but worth it IMHO.

Bump minimum PHP version to 7.3
7.2 active support ended 4 months ago, there is no need to support it anymore. This also allows us to use PHP 7.3 features like JSON_THROW_ON_ERROR.
Update composer dependencies
Because we are now using 7.3, we can update a few dependencies to their latest version.
Use strict comparison
Fix docblocks (these were found with phpstan at max level)
Use JSON_THROW_ON_ERROR and set max recursion depth
Use cast to int instead of intval

It's not often that PHP projects use phpstan and psalm. Here is a third one that I think you'll like: rector. I have added a config file (directly copied from my main project) and commands in Makefile so it's easy to use. I chose to use Docker instead of adding rector as a dependency, as it looks like you embraced docker ;)

Add rector config file
Add rector make commands (rector and rector-dry)

First use make rector-dry to see what it would modify, and if you like it use make rector so the modifications are done by rector.

Cheers,
~Nico

* Bump minimum PHP version to 7.3 * Update composer dependencies * Use strict comparison * Fix docblocks * Use JSON_THROW_ON_ERROR and set max recursion depth * Use cast to int instead of intval * Add rector config file * Add rector make commands (rector and rector-dry)

lightswitch05 · 2020-04-28T11:47:33Z

Hello Nico, thanks for the PR! Your the first person to have ever submitted a PR to this project. I’m glad you found it interesting enough to warrant your time. I really appreciate feedback from the community through tickets and PRs.

I haven’t had time to review the changes yet (on mobile), but there is one issue that immediately stands out from your description. One of the project goals listed in the README:

Runtime support for the oldest supported version of PHP. If you are using this tool with an unsupported version of PHP, then you already have all the answers that this tool can give you: Yes, you have vulnerabilities and are out of date. Of course that is just for the run-time, it is still the goal of this project to supply information about any reasonable version of PHP.

Since PHP 7.2 is still receiving security updates, then it is one of the goals of this project to continue supporting it. Looks like it will continue receiving security updates till November of this year.

I’ll review the rest of your change set as soon as I get to a computer. Thank you again for taking the time and interest in this project to review the code and submit changes! 🍻

NicolasCARPi · 2020-04-28T11:52:11Z

Ok, then I can make another PR without going to 7.3 :)

lightswitch05

The changes look good overall. Of course, 7.2 support has to be maintained for now due to project goals. Besides the version bump, the only change that stood out to me is:

$this->rules->latestVersion === []

I'm not sure why it would be an empty array? Everything else looks good, a few changes highlight some poor coding decisions on my part, but that is my own fault.

lightswitch05 · 2020-04-28T12:46:42Z

src/Application.php

    {
        $this->auditVersion = PhpVersion::fromString($phpVersion);
-        if (!$this->auditVersion) {
+        if ($this->auditVersion === null) {


lightswitch05 · 2020-04-28T12:47:54Z

src/Application.php

        $majorAndMinor = $this->auditVersion->getMajorMinorVersionString();
        $maxVersion = PhpVersion::fromString($majorAndMinor . ".9999");
        foreach($this->rules->releases as $versionString => $release) {
-            /** @var PhpRelease $release */


these are in-line type hints, not sure why they were removed?

because the line under doesn't contain the var I guess

to me it looked weirdly placed TBH.

I agree, it is weird. Also, I've never seen it done anywhere else. Leave it out 👍

lightswitch05 · 2020-04-28T12:49:43Z

src/Application.php

    public function getLatestVersion(): string
    {
-        if (!$this->rules->latestVersion) {
+        if ($this->rules->latestVersion === []) {


this is kinda surprising. Would it not be null if it was unknown?

I don't know. I would trust the analyzer on this one. But to be safe I kept it in its original form for the next PR.

lightswitch05 · 2020-04-28T12:52:07Z

src/CachedDownload.php

    {
        $html = self::download($url);
-        return json_decode($html);
+        return json_decode($html, false, 512, JSON_THROW_ON_ERROR);


Looks like JSON_THROW_ON_ERROR is 7.3 only, so that will have to wait, but I like the idea.

lightswitch05 · 2020-04-28T12:52:47Z

src/CveId.php

-        $this->year = intval($matches[1]);
-        $this->sequenceNumber = intval($matches[2]);
+        $this->year = (int) $matches[1];
+        $this->sequenceNumber = (int) $matches[2];


lightswitch05 · 2020-04-28T12:57:16Z

src/Parsers/ChangelogParser.php

        foreach ($dom->getElementsByTagName('section') as $sectionTag) {
            $versionString = $sectionTag->getAttribute('id');
-            if (!$version = PhpVersion::fromString($versionString)) {
+            if (($version = PhpVersion::fromString($versionString)) === null) {


yikes, this is hard to read. I should have never put the assignment in the if statement to begin with. I understand you are just going through making the comparisons strict, this change just highlights my already bad code.

Should just move the assignment outside of the if statement, and then the if could read simple $version === null

Yes, I saw that you are using a lot of if ($var = expression) which I personally never use. Getting the assignement out sounds like the way to go.

lightswitch05 · 2020-04-28T12:58:07Z

src/Parsers/NvdFeedParser.php

            return null;
        }
-        if(!$id = CveId::fromString($cveItem->cve->CVE_data_meta->ID)){
+        if(($id = CveId::fromString($cveItem->cve->CVE_data_meta->ID)) === null){


Gross. I need to stop putting assignments within if statements.

lightswitch05 · 2020-04-28T13:00:40Z

src/Parsers/NvdFeedParser.php

        if (isset($cveItem->impact->baseMetricV3->cvssV3->baseScore)) {
            $baseScore = $cveItem->impact->baseMetricV3->cvssV3->baseScore;
-        } else if (isset($cveItem->impact->baseMetricV2->cvssV2->baseScore)) {
+        } elseif (isset($cveItem->impact->baseMetricV2->cvssV2->baseScore)) {


Huh, interesting. 👍

only in PHP you can write an incorrect language construct and it still works 🤷‍♂️

lightswitch05 · 2020-04-28T13:02:10Z

src/PhpVersion.php

        $preReleaseVersion = null;
        if($preReleaseType = self::normalizeReleaseType($matches[4])) {
-            $preReleaseVersion = intval($matches[5]);
+            $preReleaseVersion = (int) $matches[5];


NicolasCARPi · 2020-04-28T14:51:34Z

I'll just close this and make another PR, it'll be easier for me to start from a fresh branch ;)

lightswitch05 suggested changes Apr 28, 2020

View reviewed changes

lightswitch05 added the enhancement New feature or request label Apr 28, 2020

NicolasCARPi closed this Apr 28, 2020

Update minimum PHP version to 7.3 and other fixes #30

Update minimum PHP version to 7.3 and other fixes #30

Uh oh!

Conversation

NicolasCARPi commented Apr 28, 2020

Uh oh!

lightswitch05 commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasCARPi commented Apr 28, 2020

Uh oh!

lightswitch05 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightswitch05 Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasCARPi commented Apr 28, 2020

Uh oh!

Uh oh!

lightswitch05 commented Apr 28, 2020 •

edited

Loading

lightswitch05 Apr 28, 2020 •

edited

Loading