Skip to content

Player Counter Queries #119

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.

Sign up for GitHub

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

Closed
AdmiralScoob wants to merge 2 commits into from
Closed

Player Counter Queries #119

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eebd989
Update GameQuery.php
AdmiralScoob Apr 5, 2026
29cbb83
Fix IP alias resolution in GameQuery
AdmiralScoob Apr 5, 2026
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
4 changes: 2 additions & 2 deletions player-counter/src/Models/GameQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function runQuery(Allocation $allocation): ?array
return null;
}

$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;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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 = is_ipv6($ip) ? '[' . $ip . ']' : $ip;
Comment on lines +42 to 43
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +42 to 43
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +42 to 43
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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:

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.


/** @var QueryTypeService $service */
Expand All @@ -54,7 +54,7 @@ public static function canRunQuery(?Allocation $allocation): bool
return false;
}

$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;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


return !in_array($ip, ['0.0.0.0', '::']);
Comment on lines +57 to 59
Copy link

Copilot AI Apr 5, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
}
Expand Down
Loading