Conversation
📝 WalkthroughWalkthroughThe IP/host selection logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the player-counter query logic to support allocation aliases that are hostnames (non-IP container aliases) by resolving the alias to an IP address before running queries.
Changes:
- Resolve
Allocation::$aliasviagethostbyname()whenplayer-counter.use_aliasis enabled. - Apply the resolved IP in both
runQuery()andcanRunQuery()selection logic.
| $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip; | ||
| $ip = is_ipv6($ip) ? '[' . $ip . ']' : $ip; |
There was a problem hiding this comment.
gethostbyname($allocation->alias) is called twice in the same expression, which performs duplicate DNS lookups and could even return different results if DNS changes between calls. Resolve the hostname once into a local variable and reuse it for both the is_ip() check and the selected $ip value.
| $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip; | ||
|
|
||
| return !in_array($ip, ['0.0.0.0', '::']); |
There was a problem hiding this comment.
canRunQuery() now performs a DNS lookup via gethostbyname(). If this method is used in lists/loops (e.g., rendering tables of servers/allocations), it can block request handling and add significant latency. Consider keeping canRunQuery() as a cheap check (e.g., only validate placeholder IPs / presence of alias) and perform DNS resolution in runQuery() (or cache the resolved value).
| $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip; | ||
| $ip = is_ipv6($ip) ? '[' . $ip . ']' : $ip; |
There was a problem hiding this comment.
gethostbyname() only resolves IPv4 (A) records. If an allocation alias resolves exclusively to an IPv6 (AAAA) address, gethostbyname() returns the hostname unchanged, is_ip() will be false, and the code will fall back to $allocation->ip—which may reintroduce the original “alias not usable” issue for IPv6-only setups. Consider using a resolver that supports AAAA (e.g., dns_get_record() for A/AAAA) and then applying the existing IPv6 bracket logic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@player-counter/src/Models/GameQuery.php`:
- Line 57: The line currently does redundant DNS lookups with
gethostbyname($allocation->alias) and duplicates logic used elsewhere; extract a
private helper (e.g., resolveAllocationIp or resolveAliasIp) that accepts
$allocation (or alias and fallback ip) and implements: check
config('player-counter.use_alias'), call gethostbyname only once, validate with
is_ip, and return the resolved IP or $allocation->ip; then replace both inline
usages (including the occurrence at line 42) with calls to that helper to
centralize logic and avoid double lookups.
- Around line 42-43: The code uses gethostbyname() to resolve $allocation->alias
which only returns IPv4, so IPv6 aliases won't be detected and fall back to
$allocation->ip; replace the gethostbyname() call with an IPv6-aware resolution
(e.g., check DNS_AAAA records via dns_get_record($allocation->alias, DNS_AAAA)
or use socket_addrinfo_lookup($allocation->alias)) and pick the first valid
IPv6/IPv4 address, then run the existing is_ip() and is_ipv6() logic to set $ip
(keeping the IPv6 bracket formatting). Ensure you update the resolution logic
where $ip is assigned (the lines referencing gethostbyname, is_ip, is_ipv6 and
$allocation->alias) so it handles both A and AAAA responses.
- Line 42: The current expression calls gethostbyname($allocation->alias) twice
causing redundant DNS lookups; compute and store the resolved value once (e.g.
$resolved = gethostbyname($allocation->alias)) and then use that cached
$resolved in the conditional is_ip(...) and in the ternary result when setting
$ip, keeping the existing config('player-counter.use_alias') check and falling
back to $allocation->ip when not an IP.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9160de54-a8cd-4fdc-86f8-6dbf0640b966
📒 Files selected for processing (1)
player-counter/src/Models/GameQuery.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
| $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip; | ||
| $ip = is_ipv6($ip) ? '[' . $ip . ']' : $ip; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does PHP gethostbyname support IPv6 addresses?
💡 Result:
No, PHP's gethostbyname does not support returning IPv6 addresses. It is explicitly documented as returning the IPv4 address corresponding to a given Internet host name. On failure to resolve an IPv4 address, it returns the unmodified hostname string. The official PHP manual for current versions (PHP 8+) states: "gethostbyname — Get the IPv4 address corresponding to a given Internet host name" and "Returns the IPv4 address or a string containing the unmodified hostname on failure." This documentation has been consistent across PHP 4, 5, 7, and 8. User notes and community workarounds confirm it does not query AAAA (IPv6) records; custom functions using dns_get_record(DNS_AAAA) are recommended for IPv6. Long-standing feature requests and bugs (e.g., PHP bug #49493 since 2009, doc bug #33170) highlight the lack of IPv6 support, with no resolution in mainline PHP as of 2026. Recent internal changes (e.g., PR #15555 in 2024 converting to getaddrinfo) modernize the underlying implementation but preserve the IPv4-only return behavior to maintain compatibility, as noted in discussions. For IPv6 resolution, use dns_get_record($host, DNS_AAAA) or socket_addrinfo_lookup (PHP 7.2+).
Citations:
- 1: https://www.php.net/manual/en/function.gethostbyname.php
- 2: https://www.php.net/function.gethostbyname
- 3: https://durak.org/sean/pubs/software/php-8.4.0/function.gethostbyname.html
- 4: https://bugs.php.net/bug.php?id=49493
- 5: https://www.php.net/manual/vote-note.php?id=70936&page=function.gethostbyname&vote=down
- 6: https://forums.phpfreaks.com/topic/157824-gethostbyname-for-ipv6/
- 7: https://bugs.php.net/bug.php?id=33170
- 8: getaddrinfo conversion php/php-src#15555
gethostbyname() does not support IPv6 alias resolution.
PHP's gethostbyname() only returns IPv4 addresses. If the alias resolves to an IPv6 address, it returns the hostname unchanged, which fails the is_ip() check and falls back to $allocation->ip. Since line 43 handles IPv6 formatting, consider using dns_get_record($allocation->alias, DNS_AAAA) or socket_addrinfo_lookup() to support IPv6 alias resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@player-counter/src/Models/GameQuery.php` around lines 42 - 43, The code uses
gethostbyname() to resolve $allocation->alias which only returns IPv4, so IPv6
aliases won't be detected and fall back to $allocation->ip; replace the
gethostbyname() call with an IPv6-aware resolution (e.g., check DNS_AAAA records
via dns_get_record($allocation->alias, DNS_AAAA) or use
socket_addrinfo_lookup($allocation->alias)) and pick the first valid IPv6/IPv4
address, then run the existing is_ip() and is_ipv6() logic to set $ip (keeping
the IPv6 bracket formatting). Ensure you update the resolution logic where $ip
is assigned (the lines referencing gethostbyname, is_ip, is_ipv6 and
$allocation->alias) so it handles both A and AAAA responses.
| } | ||
|
|
||
| $ip = config('player-counter.use_alias') && is_ip($allocation->alias) ? $allocation->alias : $allocation->ip; | ||
| $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip; |
There was a problem hiding this comment.
Double gethostbyname() call causes redundant DNS lookups and potential inconsistency.
gethostbyname($allocation->alias) is called twice—once in the condition and once in the result. This triggers two DNS queries per invocation, which is inefficient and could yield inconsistent results if DNS changes between calls.
🔧 Proposed fix: cache the resolved hostname
- $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip;
+ $resolvedAlias = $allocation->alias ? gethostbyname($allocation->alias) : null;
+ $ip = config('player-counter.use_alias') && is_ip($resolvedAlias) ? $resolvedAlias : $allocation->ip;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@player-counter/src/Models/GameQuery.php` at line 42, The current expression
calls gethostbyname($allocation->alias) twice causing redundant DNS lookups;
compute and store the resolved value once (e.g. $resolved =
gethostbyname($allocation->alias)) and then use that cached $resolved in the
conditional is_ip(...) and in the ternary result when setting $ip, keeping the
existing config('player-counter.use_alias') check and falling back to
$allocation->ip when not an IP.
| } | ||
|
|
||
| $ip = config('player-counter.use_alias') && is_ip($allocation->alias) ? $allocation->alias : $allocation->ip; | ||
| $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip; |
There was a problem hiding this comment.
Same double DNS lookup issue and duplicated logic.
This line has the same redundant gethostbyname() calls as line 42. Consider extracting a shared helper method to avoid duplication and ensure consistent behavior.
🔧 Proposed fix: extract helper method and cache DNS result
Add a private helper method and refactor both usages:
+ private static function resolveIp(Allocation $allocation): string
+ {
+ if (config('player-counter.use_alias') && $allocation->alias) {
+ $resolved = gethostbyname($allocation->alias);
+ if (is_ip($resolved)) {
+ return $resolved;
+ }
+ }
+
+ return $allocation->ip;
+ }
+
/** `@return` ?array{hostname: string, map: string, current_players: int, max_players: int, players: ?array<array{id: string, name: string}>} */
public function runQuery(Allocation $allocation): ?array
{
if (!static::canRunQuery($allocation)) {
return null;
}
- $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip;
+ $ip = static::resolveIp($allocation);
$ip = is_ipv6($ip) ? '[' . $ip . ']' : $ip; public static function canRunQuery(?Allocation $allocation): bool
{
if (!$allocation) {
return false;
}
- $ip = config('player-counter.use_alias') && is_ip(gethostbyname($allocation->alias)) ? gethostbyname($allocation->alias) : $allocation->ip;
+ $ip = static::resolveIp($allocation);
return !in_array($ip, ['0.0.0.0', '::']);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@player-counter/src/Models/GameQuery.php` at line 57, The line currently does
redundant DNS lookups with gethostbyname($allocation->alias) and duplicates
logic used elsewhere; extract a private helper (e.g., resolveAllocationIp or
resolveAliasIp) that accepts $allocation (or alias and fallback ip) and
implements: check config('player-counter.use_alias'), call gethostbyname only
once, validate with is_ip, and return the resolved IP or $allocation->ip; then
replace both inline usages (including the occurrence at line 42) with calls to
that helper to centralize logic and avoid double lookups.
|
Thanks but the |
Modified "runQuery" and "canRunQuery" to use gethostbyname with the allocation alias to fix issues with non-IP container aliases.
Summary by CodeRabbit