Issue/91. Handle patch correctly. #98

slash3b · 2019-12-13T17:02:31Z

TLDR: priority fix and added some more tests

Fixed major flaw — turned out within extent of same version patch flag is of the highest priority.
Priority sequence should be the following:

1.1-alpha < 1.1-beta < 1.1-rc < 1.1-stable < 1.1 < 1.1-p

diff between old version and fixed one:

slash3b@Nostromo ~/P/SecurityAdvisoriesBuilder (issue/91)> diff -u old_composer.json build/composer.json
--- old_composer.json   2019-12-13 18:25:18.956262258 +0200
+++ build/composer.json 2019-12-13 18:27:19.958957880 +0200
@@ -75,7 +75,7 @@
         "league/commonmark": "<0.18.3",
         "magento/magento1ce": "<1.9.4.3",
         "magento/magento1ee": ">=1,<1.14.4.3",
-        "magento/product-community-edition": ">=2,<2.2.10|>=2.3,<2.3.2",
+        "magento/product-community-edition": ">=2,<2.2.10|>=2.3,<2.3.2-p.2",
         "monolog/monolog": ">=1.8,<1.12",
         "namshi/jose": "<2.2",
         "onelogin/php-saml": "<2.10.4",

…h has the highest rank

… to use mixed

Ocramius

Overall looking good, but we need checks among -p1, -p2 and -p3 .

@nikolaposa you might be interested in this sort of patch too 👍

Ocramius · 2019-12-14T09:48:06Z

test/RoaveTest/SecurityAdvisories/ComponentTest.php

+    }
+
+    /**
+     * @return mixed


This js an array<string, array<int, string|array<string, array<string, array<string>>>> or such 😵😵😵

I did not know how to correct this 😓
Is @return mixed okay ?

It's a test data provider: we're good for now 👍

test/RoaveTest/SecurityAdvisories/VersionTest.php

src/Roave/SecurityAdvisories/Version.php

complex than I expected. Apart from designating Flag class for all the logic, spaceship operator logic was ditched — it became too complex so insted we just use boolean. PRIORITY map in Flag class contains null, looks ugly, but allowed to throw away lots of checks and makes flag comparison more obvious and "natural" I would say.

nikolaposa · 2019-12-14T11:13:01Z

Thanks for the ping @Ocramius. 👍

On a different note, these days I'm working on Version 4.0.0, lots of backward-incompatible improvements in it (https://github.com/nikolaposa/version/blob/master/CHANGELOG.md#unreleased). 😒 BackwardCompatibilityCheck library is by far the biggest dependent so I'll help you with migrating, expect a PR soon!

slash3b · 2019-12-14T17:20:47Z

src/Roave/SecurityAdvisories/Flag.php

+    private const PRIORITY = [
+        'patch'     => 5,
+        'p'         => 5,
+        null        => 4, // special case of clean version, e.g. 1.2.3


I guess it looks a bit ugly, but when I added this null in the map, it helped to ditch so many useless checks.
So I think it is ugly and... elegant at the same time 😅

null is not a valid array key. This is probably adding the empty string instead.

🤔 I'd say it actually is a valid key, at least I'm being able to get value from array by null

But you got me thinking, and yes null is not very good key. I'm going to change it to string value with some meaning attached.

Thanks!

In the end empty string was the best variant. I did push update.

slash3b · 2019-12-14T17:22:41Z

src/Roave/SecurityAdvisories/Version.php

    {
+        $this->versionNumbers = self::removeTrailingZeroes(...array_map('intval', explode('.', $matches['version'])));


just moved this logic from fromString static method to unload it a little bit

slash3b · 2019-12-14T17:24:23Z

src/Roave/SecurityAdvisories/Version.php

    }

    public function equalTo(self $other) : bool
    {
-        return $other->versionNumbers === $this->versionNumbers &&
-            $this->stabilityEqualTo($other);
+        return $other->versionNumbers === $this->versionNumbers


simplified logic, no need for stabilityEqualTo

slash3b · 2019-12-14T17:30:21Z

test/RoaveTest/SecurityAdvisories/VersionTest.php

+            // compare with version lower
+            ['2.3.2-p2', '2.3.1', true, false],
+            // compare two patches but with additional stability versions like 1-p1 and 1-p2 so the p2 will be greater
+            ['2.3.2-p1', '2.3.2-p2', false, true],


@Ocramius I do this p1 vs p2 comparison here.
Also I have to admit it starts to get out of hand, data providers grow and it is a bit hard to navigate.

Yeah, but that's combinatory logic with edge cases 🤷‍♀️

The good thing is that I'm not (currently) aware of further edge cases

slash3b · 2019-12-14T17:34:26Z

It is ready for review 🤞

… php casts null to empty string anyway

Ocramius

LGTM, excellent work @slash3b!

slash3b · 2019-12-17T09:23:13Z

@Ocramius thank you!
I'm so glad I could help ^^

Ocramius · 2019-12-17T09:47:44Z

Seems fixed downstream 💃 Roave/SecurityAdvisories@72ffba7#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R78

nikolaposa · 2019-12-22T21:14:32Z

@slash3b What does -p actually denote? Is that some custom pre-release identifier? It definitely isn't Semver-compliant because the correct order should be:

1.1-alpha < 1.1-beta < 1.1-p < 1.1-rc < 1.1-stable < 1.1

Precedence for two pre-release versions with the same major, minor, and patch version MUST be determined by comparing each dot separated identifier from left to right until a difference is found as follows: identifiers consisting of only digits are compared numerically and identifiers with letters or hyphens are compared lexically in ASCII sort order.

Ocramius · 2019-12-22T21:27:46Z

-p is for -patch, so 1.2.0-p1 is a bit higher than 1.2.0. I was as confused as you when Magento decided to release with this modifier 🤷‍♀️

nikolaposa · 2019-12-22T21:33:09Z

Oh no.

slash3b added 6 commits December 13, 2019 17:03

issue/91. Changed mode of comparisor for stabilities. Turned out patc…

06b4b61

…h has the highest rank

issue/91. Fixes for tests

2612506

issue/91. All tests are fixed!

edda98d

issue/91. Added real case test scenario for magento

708a331

issue/91. fucking pipeline is killin me!

85b09b0

issue/91. phpstan wont let me use simple array return type, so I have…

3ca492b

… to use mixed

slash3b force-pushed the issue/91 branch from ac89bd7 to 3ca492b Compare December 13, 2019 21:08

Ocramius requested changes Dec 14, 2019

View reviewed changes

slash3b added 2 commits December 14, 2019 12:49

issue/91. todo comment

e618f39

slash3b added 2 commits December 14, 2019 18:55

issue/91. bits and pieces just to deal with the pipline from hell

940e5b1

issue/91. tiny phpstan warning fix

c227930

slash3b commented Dec 14, 2019

View reviewed changes

slash3b marked this pull request as ready for review December 14, 2019 17:31

slash3b requested a review from Ocramius December 14, 2019 17:35

slash3b mentioned this pull request Dec 14, 2019

-p\d+ and -patch\d+ releases aren't merged together correctly with existing stable releases #91

Closed

issue/91. special flag marker for 'clean' version changed to null, as…

4a01b0f

… php casts null to empty string anyway

Ocramius approved these changes Dec 17, 2019

View reviewed changes

Ocramius merged commit 1d40c01 into Roave:master Dec 17, 2019

Ocramius added the enhancement label Dec 17, 2019

Ocramius self-assigned this Dec 17, 2019

Ocramius mentioned this pull request Jan 21, 2021

propel: 2.0.0-alpha11 FriendsOfPHP/security-advisories#511

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/91. Handle patch correctly. #98

Issue/91. Handle patch correctly. #98

slash3b commented Dec 13, 2019 •

edited

Loading

Ocramius left a comment

Ocramius Dec 14, 2019

slash3b Dec 14, 2019

Ocramius Dec 17, 2019

nikolaposa commented Dec 14, 2019

slash3b Dec 14, 2019

stof Dec 16, 2019

slash3b Dec 16, 2019

slash3b Dec 16, 2019

slash3b Dec 14, 2019

slash3b Dec 14, 2019

slash3b Dec 14, 2019

Ocramius Dec 17, 2019

slash3b commented Dec 14, 2019

Ocramius left a comment

slash3b commented Dec 17, 2019

Ocramius commented Dec 17, 2019

nikolaposa commented Dec 22, 2019 •

edited

Loading

Ocramius commented Dec 22, 2019

nikolaposa commented Dec 22, 2019

		{
		$this->versionNumbers = self::removeTrailingZeroes(...array_map('intval', explode('.', $matches['version'])));

Issue/91. Handle patch correctly. #98

Issue/91. Handle patch correctly. #98

Conversation

slash3b commented Dec 13, 2019 • edited Loading

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolaposa commented Dec 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slash3b commented Dec 14, 2019

Ocramius left a comment

Choose a reason for hiding this comment

slash3b commented Dec 17, 2019

Ocramius commented Dec 17, 2019

nikolaposa commented Dec 22, 2019 • edited Loading

Ocramius commented Dec 22, 2019

nikolaposa commented Dec 22, 2019

slash3b commented Dec 13, 2019 •

edited

Loading

nikolaposa commented Dec 22, 2019 •

edited

Loading