Issue 32 - php8.1 support #33
Conversation
| * | ||
| * @return \Leth\IPAddress\IP\Address|\Leth\IPAddress\IPv4\Address|\Leth\IPAddress\IPv6\Address An instance of a subclass of IP\Address; either IPv4\Address or IPv6\Address | ||
| */ | ||
| public static function factory(IP\Address|int|string|\Math_BigInteger $address): IP\Address|Ipv4\Address|IPv6\Address |
There was a problem hiding this comment.
Misaligned indentation. Sorry, I legally have to point out one formatting thing during a code review.
There was a problem hiding this comment.
For all code I'd like to use PHPFixer.
What do You think about it?
There was a problem hiding this comment.
I can't comment on PHPFixer itself, but I'd strongly encourage adopting automatic tooling to enforce code style; it saves everyone time, avoids a lot of nitpicky feedback which is not fun for anyone, and usually makes code easier to read and interpret correctly!
There was a problem hiding this comment.
Yeah same, but I think that warrants its own PR.
| * | ||
| * @return \Leth\IPAddress\IPv4\NetworkAddress|\Leth\IPAddress\IP\NetworkAddress|\Leth\IPAddress\IPv6\NetworkAddress | ||
| */ | ||
| public static function factory(NetworkAddress|Address|string $address, int|string|null $cidr = NULL): IPv4\NetworkAddress|NetworkAddress|IPv6\NetworkAddress |
There was a problem hiding this comment.
a) the indentation
b) Just musing, but would this be an opportunity to require that $cidr be an int if present? For both v4 and v6 it'll always be in the bounds of 0-128, so there's no real reason to allow a string, and it would save validation and an (int) cast below. This PR is already a BC break and new major release, so this might be the time.
There was a problem hiding this comment.
Okay ignore my comment regarding the parameter types, that can/should be a different PR.
Please do fix the indentation, though.
There was a problem hiding this comment.
The same situation. What do you think about using PHPFixer?
There was a problem hiding this comment.
I'm not opposed to adding some tooling for style/formatting, but I think that should be its own PR.
|
Maybe it's worth making a 2.x branch here, as an opportunity to collect other non-compatibility-related breaking changes that would improve the API. @leth Are you okay if I start that process? |
|
Okay I went ahead and started up a 2.0-dev branch to collect any other backwards-incompatible changes to prep for a 2.0 release, and updated this PR's base to be that branch. |
No objections from me, sounds wise if you have some ideas, or expect some more improvements to make it more idiomatic php8 :) |
|
Looks like https://github.com/leth/PHP-IPAddress/blob/master/.github/workflows/test.yml also needs to be updated to reflect the current version support. |
|
I've created a branch with (hopefully) the required changes to the github actions config. |
|
Actually, I'll push those into the 2.0-dev branch, to see if they fix the CI for this PR :) |
|
Okay, looks like that doesn't affect the CI for this PR :( |
|
I figured it'd be easiest if I did that, and worked out a way to do it! I sorted out the tabs vs spaces issues too! |
3 participants
#32
I did my best to get PHP8.1 support and fix some bugs.
@leth please review this PR.