Rename isPublicUse() to isGloballyReachable()#107
Conversation
Conform to official wording from the IANA special-purpose address registries; "Public Use" does not appear in them. Keep isPublicUse() as a deprecated alias.
Greptile SummaryThis PR renames
Confidence Score: 4/5Safe to merge; the rename is mechanical and correct, with no logic changes to the underlying routing checks. The delegation chain is straightforward and the bitmask values in test data are unchanged, so the core behaviour is intact. Two small gaps remain: the src/Version/IPv4.php, src/Version/IPv6.php, and src/Version/Multi.php each need the Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant IPv4
participant IPv6
participant Multi
Note over Caller,Multi: New primary method
Caller->>IPv4: isGloballyReachable()
IPv4-->>Caller: bool (full logic here)
Caller->>IPv6: isGloballyReachable()
IPv6-->>Caller: bool (full logic here)
Caller->>Multi: isGloballyReachable()
alt isEmbedded()
Multi->>IPv4: isGloballyReachable()
IPv4-->>Multi: bool
else
Multi->>IPv6: isGloballyReachable()
IPv6-->>Multi: bool
end
Multi-->>Caller: bool
Note over Caller,Multi: Deprecated alias
Caller->>IPv4: isPublicUse()
IPv4->>IPv4: isGloballyReachable()
IPv4-->>Caller: bool
Caller->>IPv6: isPublicUse()
IPv6->>IPv6: isGloballyReachable()
IPv6-->>Caller: bool
Caller->>Multi: isPublicUse()
Multi->>Multi: isGloballyReachable()
Multi-->>Caller: bool
Reviews (1): Last reviewed commit: "feature: ✨ rename isPublicUse() to isGlo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public function isPublicUse(): bool | ||
| { | ||
| return $this->isGloballyReachable(); | ||
| } |
There was a problem hiding this comment.
@deprecated on concrete implementations
The @deprecated annotation exists on IpInterface::isPublicUse() but is absent from the concrete implementations in IPv4, IPv6, and Multi. IDEs and static analysers (PHPStan, Psalm) resolve deprecation warnings from the declared type at the call site. When user code holds a variable typed as the concrete class rather than IpInterface, the tooling will silently accept isPublicUse() calls without any deprecation notice, defeating the purpose of the annotation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| /** | ||
| * @test | ||
| * @dataProvider \Darsyn\IP\Tests\DataProvider\IPv4::getPublicUseIpAddresses() | ||
| * @dataProvider \Darsyn\IP\Tests\DataProvider\IPv4::getGloballyReachableIpAddresses() | ||
| */ | ||
| #[PHPUnit\Test] | ||
| #[PHPUnit\DataProviderExternal(IPv4DataProvider::class, 'getPublicUseIpAddresses')] | ||
| public function testIsPublicUse(string $value, bool $isPublicUse): void | ||
| #[PHPUnit\DataProviderExternal(IPv4DataProvider::class, 'getGloballyReachableIpAddresses')] | ||
| public function testIsGloballyReachable(string $value, bool $isGloballyReachable): void | ||
| { | ||
| $ip = IP::factory($value); | ||
| $this->assertSame($isPublicUse, $ip->isPublicUse()); | ||
| $this->assertSame($isGloballyReachable, $ip->isGloballyReachable()); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
isPublicUse() alias
All three test classes (IPv4Test, IPv6Test, MultiTest) had their testIsPublicUse methods renamed to testIsGloballyReachable, which now only exercises the new method. The deprecated isPublicUse() delegate is completely untested — a future refactor that accidentally breaks the delegation (e.g., returning a hardcoded value) would go undetected. A minimal parameterised test that calls isPublicUse() and asserts it matches isGloballyReachable() would guard the alias.
1 participant
Conform to official wording from the IANA special-purpose address registries; Public Use does not appear in them. Keep
isPublicUse()as a deprecated alias.