Conversation
thomasnordahl-dk
left a comment
There was a problem hiding this comment.
I appreciate the boyscout rule, but adding return types and the default makes this backwards incompatible. Let's keep it light for the PHP 8.4 upgrade, and not do major versions here.
src/SessionData.php
Outdated
| public function __construct( | ||
| private string $client_session_id, | ||
| private array $data, | ||
| private bool $is_new = false |
There was a problem hiding this comment.
| private bool $is_new = false | |
| private bool $is_new |
Adding it as a default is backwards incompatible
There was a problem hiding this comment.
It already has a default in line 40, so it doesn't have a difference.
Besides that, I am not sure how far backwards compatible we are talking about. composer.json says minimum php 8.0 already and everything passes in 8.0 environment.
There was a problem hiding this comment.
It wasn't default in the constructor signature, which is the breaking part :-)
original: public function __construct(string $client_session_id, array $data, bool $is_new)
There was a problem hiding this comment.
Because it is public, we should stay strict - we can cut corners on private repositories because we know how we extend and when a b\c break is an issue or not.
src/Session.php
Outdated
| * @return void | ||
| */ | ||
| public function clear(); | ||
| public function clear(): void; |
There was a problem hiding this comment.
I checked in 3v4l.org and I think the void return type isn't an issue on extension. This works fine:
<?php
class Foo
{
public function myMethod()
{
echo 'hello';
}
}
class Bar extends Foo
{
public function myMethod(): void
{
echo 'hola';
}
}
new Bar()->myMethod();There was a problem hiding this comment.
It is not internally, because SessionData has the return types as well. In your example Foo should be an interface and it should have the return type, not Bar. In that case it throws a fatal error:
Fatal error: Declaration of SessionData::clear() must be compatible with Session::clear(): void in /in/JcVqe on line 8
So, the return types should be removed from the interface and SessionData if we want to keep them compatible.
97ec1ae to
b5d9d04
Compare
Tests did not fail at all, but there were some areas that could be improved, so I just did that.