Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Extract Version #872

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions include/branches.inc
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
<?php

require_once __DIR__ . '/../src/autoload.php';
include_once __DIR__ . '/releases.inc';
include_once __DIR__ . '/version.inc';

use phpweb\Release\Version;

/* Branch overrides. For situations where we've changed the exact dates for a
* branch's active support and security fix EOLs, these can be reflected here.
*
Expand Down Expand Up @@ -85,13 +89,18 @@ function format_interval($from, DateTime $to) {
return $eolPeriod;
}

function version_number_to_branch(string $version): ?string {
$parts = explode('.', $version);
if (count($parts) > 1) {
return "$parts[0].$parts[1]";
function version_number_to_branch(string $value): ?string {
Copy link
Contributor Author

@localheinz localheinz Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could eventually disappear, and is only a starting point for using Version - also see #871.

try {
$version = Version::fromString($value);
} catch (\InvalidArgumentException) {
return null;
}

return null;
return sprintf(
'%s.%s',
$version->major()->toString(),
$version->minor()->toString(),
);
}

function get_all_branches() {
Expand Down
32 changes: 32 additions & 0 deletions src/Release/Major.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare(strict_types=1);

namespace phpweb\Release;

final readonly class Major
{
private function __construct(private string $value)
{
}

/**
* @throws \ValueError
*/
public static function fromString(string $value): self
{
if (1 !== preg_match('/^(0|[1-9]\d*)$/', $value)) {
throw new \ValueError(\sprintf(
'Value "%s" does not appear to be a valid value for a major version.',
$value,
));
}

return new self($value);
}

public function toString(): string
{
return $this->value;
}
Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is completely unnecessary and also can be easily confused with the magic method __toString. This is the same as making the property public and directly accessing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a matter of preference - when I allow constructing an object from a simple type, I typically use a named constructor fromSimpleType() and a corresponding method toSimpleType(). This allows to construct an object from and cast it to other simple types when necessary, and hides the internals.

I have written about Naming constructors in PHP, where I attempt to explain my reasoning.

Also see https://github.com/php/web-php/pull/872/files#r1418528779.

}
32 changes: 32 additions & 0 deletions src/Release/Minor.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare(strict_types=1);

namespace phpweb\Release;

final readonly class Minor
{
private function __construct(private string $value)
{
}

/**
* @throws \ValueError
*/
public static function fromString(string $value): self
{
if (1 !== preg_match('/^(0|[1-9]\d*)$/', $value)) {
throw new \ValueError(\sprintf(
'Value "%s" does not appear to be a valid value for a minor version.',
$value,
));
}

return new self($value);
}

public function toString(): string
{
return $this->value;
}
}
32 changes: 32 additions & 0 deletions src/Release/Patch.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare(strict_types=1);

namespace phpweb\Release;

final readonly class Patch
{
private function __construct(private string $value)
{
}

/**
* @throws \ValueError
*/
public static function fromString(string $value): self
{
if (1 !== preg_match('/^(0|[1-9]\d*)$/', $value)) {
throw new \ValueError(\sprintf(
'Value "%s" does not appear to be a valid value for a patch version.',
$value,
));
}

return new self($value);
}

public function toString(): string
{
return $this->value;
}
}
71 changes: 71 additions & 0 deletions src/Release/Version.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php

declare(strict_types=1);

namespace phpweb\Release;

final readonly class Version
{
private function __construct(
private Major $major,
private Minor $minor,
private Patch $patch,
) {
}

public static function create(
Major $major,
Minor $minor,
Patch $patch,
): self {
return new self(
$major,
$minor,
$patch,
);
}
Comment on lines +16 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make the constructor public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using one or more public named constructors and a single private constructor in conjunction with accessors instead of public properties allows us to change the internals of the class as needed.

From the call-site it does not matter whether someone invokes a method or accesses a property, but relying on a public property makes it harder to change internals.

For example, we could change the internals as follows and would not need to make any changes to call-sites:

diff --git a/src/Release/Version.php b/src/Release/Version.php
index 47975096..6b5664fd 100644
--- a/src/Release/Version.php
+++ b/src/Release/Version.php
@@ -6,11 +6,9 @@ namespace phpweb\Release;

 final readonly class Version
 {
-    private function __construct(
-        private Major $major,
-        private Minor $minor,
-        private Patch $patch,
-    ) {
+    private function __construct(private string $value)
+    {
+
     }

     public static function create(
@@ -18,11 +16,12 @@ final readonly class Version
         Minor $minor,
         Patch $patch,
     ): self {
-        return new self(
-            $major,
-            $minor,
-            $patch,
-        );
+        return new self(sprintf(
+            '%s.%s.%s',
+            $major->toString(),
+            $minor->toString(),
+            $patch->toString(),
+        ));
     }

     /**
@@ -37,35 +36,32 @@ final readonly class Version
             ));
         }

-        return new self(
-            Major::fromString($matches['major']),
-            Minor::fromString($matches['minor']),
-            Patch::fromString($matches['patch']),
-        );
+        return new self($value);
     }

     public function major(): Major
     {
-        return $this->major;
+        $parts = explode('.', $this->value);
+
+        return Major::fromString($parts[0]);
     }

     public function minor(): Minor
     {
-        return $this->minor;
+        $parts = explode('.', $this->value);
+
+        return Minor::fromString($parts[1]);
     }

     public function patch(): Patch
     {
-        return $this->patch;
+        $parts = explode('.', $this->value);
+
+        return Patch::fromString($parts[2]);
     }

     public function toString(): string
     {
-        return sprintf(
-            '%s.%s.%s',
-            $this->major->toString(),
-            $this->minor->toString(),
-            $this->patch->toString(),
-        );
+        return $this->value;
     }
 }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer the principles of KISS and YAGNI. When it comes to value objects, they should remain as simple as possible. But there's no reason to spend too much time on this as it feels more of a stylistic choice than anything else.


/**
* @throws \ValueError
*/
public static function fromString(string $value): self
{
if (1 !== preg_match('/^(?P<major>(0|[1-9]\d*))\.(?P<minor>(0|[1-9]\d*))\.(?P<patch>(0|[1-9]\d*))$/', $value, $matches)) {
throw new \ValueError(\sprintf(
'Value "%s" does not appear to be a valid value for a version.',
$value,
));
}

return new self(
Major::fromString($matches['major']),
Minor::fromString($matches['minor']),
Patch::fromString($matches['patch']),
);
}

public function major(): Major
{
return $this->major;
}

public function minor(): Minor
{
return $this->minor;
}

public function patch(): Patch
{
return $this->patch;
}
Comment on lines +47 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need those if we have readonly properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on whether we want to stick with the internal implementation: using accessors instead of public properties has the advantage that we can change the internals any time without changing the consumers.

I can adjust, though.


public function toString(): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use the magic method __toString?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to explicitly invoke a method to cast a type to another type instead of relying on a magic method.

{
return sprintf(
'%s.%s.%s',
$this->major->toString(),
$this->minor->toString(),
$this->patch->toString(),
);
}
}
41 changes: 41 additions & 0 deletions tests/Release/Major/from-string-rejects-invalid-value.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Major::fromString() rejects invalid values
--FILE--
<?php

declare(strict_types=1);

use phpweb\Release\Major;

require_once __DIR__ . '/../../../src/autoload.php';

$values = [
'string-blank' => ' ',
'string-empty' => '',
'string-leading-zero' => '01',
'string-word' => 'foo',
];

$invalidValues = array_filter($values, static function (string $value): bool {
try {
Major::fromString($value);
} catch (\ValueError) {
return true;
}

return false;
});

var_dump($invalidValues);
?>
--EXPECT--
array(4) {
["string-blank"]=>
string(1) " "
["string-empty"]=>
string(0) ""
["string-leading-zero"]=>
string(2) "01"
["string-word"]=>
string(3) "foo"
}
17 changes: 17 additions & 0 deletions tests/Release/Major/from-string-returns-major.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
Major::fromString() returns Major
--FILE--
<?php

declare(strict_types=1);

use phpweb\Release\Major;

require_once __DIR__ . '/../../../src/autoload.php';

$major = Major::fromString('8');

var_dump($major->toString());
?>
--EXPECT--
string(1) "8"
41 changes: 41 additions & 0 deletions tests/Release/Minor/from-string-rejects-invalid-value.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Minor::fromString() rejects invalid values
--FILE--
<?php

declare(strict_types=1);

use phpweb\Release\Minor;

require_once __DIR__ . '/../../../src/autoload.php';

$values = [
'string-blank' => ' ',
'string-empty' => '',
'string-leading-zero' => '01',
'string-word' => 'foo',
];

$invalidValues = array_filter($values, static function (string $value): bool {
try {
Minor::fromString($value);
} catch (\ValueError) {
return true;
}

return false;
});

var_dump($invalidValues);
?>
--EXPECT--
array(4) {
["string-blank"]=>
string(1) " "
["string-empty"]=>
string(0) ""
["string-leading-zero"]=>
string(2) "01"
["string-word"]=>
string(3) "foo"
}
17 changes: 17 additions & 0 deletions tests/Release/Minor/from-string-returns-minor.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
Minor::fromString() returns Minor
--FILE--
<?php

declare(strict_types=1);

use phpweb\Release\Minor;

require_once __DIR__ . '/../../../src/autoload.php';

$minor = Minor::fromString('3');

var_dump($minor->toString());
?>
--EXPECT--
string(1) "3"
41 changes: 41 additions & 0 deletions tests/Release/Patch/from-string-rejects-invalid-value.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Patch::fromString() rejects invalid values
--FILE--
<?php

declare(strict_types=1);

use phpweb\Release\Patch;

require_once __DIR__ . '/../../../src/autoload.php';

$values = [
'string-blank' => ' ',
'string-empty' => '',
'string-leading-zero' => '01',
'string-word' => 'foo',
];

$invalidValues = array_filter($values, static function (string $value): bool {
try {
Patch::fromString($value);
} catch (\ValueError) {
return true;
}

return false;
});

var_dump($invalidValues);
?>
--EXPECT--
array(4) {
["string-blank"]=>
string(1) " "
["string-empty"]=>
string(0) ""
["string-leading-zero"]=>
string(2) "01"
["string-word"]=>
string(3) "foo"
}
17 changes: 17 additions & 0 deletions tests/Release/Patch/from-string-returns-patch.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
Patch::fromString() returns Patch
--FILE--
<?php

declare(strict_types=1);

use phpweb\Release\Patch;

require_once __DIR__ . '/../../../src/autoload.php';

$patch = Patch::fromString('8');

var_dump($patch->toString());
?>
--EXPECT--
string(1) "8"
Loading