src/Appwrite/Network/Validator/DNS.php (4)

1-5: Missing strict type declaration.

Per coding guidelines, PHP files should use strict type declarations where applicable. Consider adding it after the opening tag.

Suggested fix

 <?php
+
+declare(strict_types=1);
 
 namespace Appwrite\Network\Validator;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Network/Validator/DNS.php` around lines 1 - 5, Add a strict
types declaration to the top of the PHP file: insert declare(strict_types=1);
immediately after the opening <?php tag in
src/Appwrite/Network/Validator/DNS.php so the file follows project coding
guidelines; no other changes to the namespace Appwrite\Network\Validator or use
statements (e.g., Channel) are required.

---

47-47: Consider adding a timeout to pop() to prevent indefinite blocking.

If a DNS query hangs, $channel->pop() will block indefinitely. Swoole channels support a timeout parameter: $channel->pop(timeout_in_seconds). Adding a reasonable timeout would improve resilience.

Suggested fix

-            $result = $channel->pop();
+            $result = $channel->pop(30); // 30 second timeout
+            if ($result === false) {
+                // Channel closed or timeout - treat as failure
+                $this->reason = 'DNS validation timed out';
+                return false;
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Network/Validator/DNS.php` at line 47, The call to
$channel->pop() in DNS.php can block indefinitely if a DNS query hangs; update
the code where $channel->pop() is used (the DNS resolver logic) to pass a
reasonable timeout (e.g., $channel->pop($timeoutInSeconds)) and handle the
timeout case by checking for a false/empty result and returning/logging an error
or retrying accordingly so the resolver doesn't block forever.

---

35-44: Edge case: empty $dnsServers array passes validation without checking.

If $dnsServers is empty, $serverCount is 0, no coroutines spawn, and the method returns true without performing any DNS validation. While the caller (per context snippet 1) defaults to 8.8.8.8, the validator itself doesn't enforce this invariant.

Consider falling back to the parent class validation when no custom DNS servers are configured.

Suggested fix

     public function isValid(mixed $value): bool
     {
         $serverCount = \count($this->dnsServers);
+        
+        if ($serverCount === 0) {
+            return parent::isValid($value);
+        }
+        
         $channel = new Channel($serverCount);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Network/Validator/DNS.php` around lines 35 - 44, The DNS
validator currently counts $this->dnsServers and spawns coroutines with Channel
but if $this->dnsServers is empty it sets $serverCount=0, spawns nothing and
effectively returns success; fix by detecting an empty $this->dnsServers before
creating the Channel and, in that case, delegate to the parent validation (call
parent::isValid($value)) so the original/default DNS behavior is preserved; keep
the existing concurrent path (Channel, BaseDNS, isValid, validator) unchanged
for non-empty $this->dnsServers.

---

46-56: Early return leaves coroutines running unnecessarily.

When validation fails and return false is executed on line 54, remaining coroutines continue running their DNS queries. While the buffered channel prevents blocking (coroutines can push without waiting), this wastes resources by completing queries whose results are discarded.

Consider draining the channel before returning to ensure clean resource handling:

Suggested fix

         for ($i = 0; $i < $serverCount; $i++) {
             $result = $channel->pop();
             if (!$result['valid']) {
                 $failed = $result['validator'];
                 $this->count = $failed->count;
                 $this->value = $failed->value;
                 $this->reason = $failed->reason;
                 $this->records = $failed->records;
+                // Drain remaining results to allow proper cleanup
+                for ($j = $i + 1; $j < $serverCount; $j++) {
+                    $channel->pop();
+                }
                 return false;
             }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Appwrite/Network/Validator/DNS.php` around lines 46 - 56, The early
return inside the for loop after detecting a failed result leaves the other
coroutines running and their results buffered; modify the code around the loop
that iterates using $serverCount and $channel->pop() so that when a failure is
detected (the branch that sets $failed,
$this->count/$this->value/$this->reason/$this->records and is about to return
false) you first drain the channel of the remaining expected responses (e.g.,
loop to pop the remaining ($serverCount - $i - 1) items or while the channel has
items) to consume and discard pending results, then return false; ensure you
reference the existing variables $serverCount, $channel, $result and $i when
implementing the drain.

tests/unit/Network/Validators/DNSTest.php (2)

13-13: Consider extracting a coroutine test helper to remove duplication.

The repeated \Swoole\Coroutine\run(function () { ... }) wrapper appears in all four tests and can be centralized for readability.

🧩 Suggested DRY refactor

 class DNSTest extends TestCase
 {
+    private function runInCoroutine(callable $test): void
+    {
+        \Swoole\Coroutine\run($test);
+    }
+
     public function testSingleDNSServer(): void
     {
-        \Swoole\Coroutine\run(function () {
+        $this->runInCoroutine(function (): void {
             $validator = new DNS('appwrite.io', Record::TYPE_CNAME, ['8.8.8.8']);
             ...
         });
     }

Also applies to: 24-24, 35-35, 50-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/Network/Validators/DNSTest.php` at line 13, Extract the repeated
\Swoole\Coroutine\run(function () { ... }) wrapper into a single helper method
on the DNSTest class (e.g., private function runCoroutine(callable $fn) or
coroutineRun(callable $fn)) and replace each inline \Swoole\Coroutine\run(...)
occurrence in the DNSTest test methods with $this->runCoroutine(function() { ...
}); ensure the helper simply delegates to \Swoole\Coroutine\run($fn) so test
bodies remain unchanged and duplication is removed.

---

16-17: Prefer boolean-specific assertions over assertEquals(false, ...).

At Line 16, Line 17, Line 27, Line 28, Line 40, Line 55, and Line 58, assertFalse(...) is clearer and avoids loose comparisons.

♻️ Suggested assertion refinement

-            $this->assertEquals(false, $validator->isValid(''));
-            $this->assertEquals(false, $validator->isValid(null));
+            $this->assertFalse($validator->isValid(''));
+            $this->assertFalse($validator->isValid(null));

-            $this->assertEquals(false, $result);
+            $this->assertFalse($result);

Also applies to: 27-28, 40-40, 55-55, 58-58

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/Network/Validators/DNSTest.php` around lines 16 - 17, Replace
loose boolean equality assertions with boolean-specific PHPUnit assertions in
DNSTest.php: swap assertEquals(false, $validator->isValid(...)) calls to
assertFalse($validator->isValid(...)) (and any assertEquals(true, ...) to
assertTrue(...) if present) for the tests that call $validator->isValid('') and
$validator->isValid(null) (and the other occurrences mentioned). Update the
assertions that reference the validator->isValid(...) calls so they use
assertFalse/assertTrue to make the tests explicit and avoid loose comparisons.

tests/unit/Network/Validators/DNSTest.php (2)

19-24: These “unit” tests are network-dependent and likely flaky.

Using public DNS servers and environment-specific resolver IPs makes results dependent on network and infra state. Consider isolating resolver behavior behind mocks/fakes for deterministic unit coverage, and keep live DNS checks in integration tests.

Also applies to: 30-35, 41-49, 57-63

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/Network/Validators/DNSTest.php` around lines 19 - 24, The tests in
DNSTest use real network resolvers (creating DNS('appwrite.io',
Record::TYPE_CNAME, ['8.8.8.8'])) which makes them flaky; change these unit
tests to use a mock/fake DNS resolver or inject a resolver dependency into the
DNS validator so you can stub responses deterministically rather than calling
real DNS. Update tests referencing DNS, Record::TYPE_CNAME, DNS::isValid and
DNS::getType to instantiate DNS with a test resolver or replace the resolver
calls with a fake that returns controlled answers; alternatively move the
existing live-resolution assertions into an integration test suite and keep unit
tests solely asserting behavior against the mocked resolver.

---

11-14: Add a skip guard for Swoole Coroutine availability to match the pattern used elsewhere in the test suite.

This test directly calls \Swoole\Coroutine\run() without checking if Coroutine is available. While ext-swoole is a required dependency, other test files (e.g., RealtimeCustomClientTest.php) already implement skip guards for coroutine usage. Use class_exists(\Swoole\Coroutine::class) to stay consistent with the existing pattern in the codebase:

Suggested patch

     private function runInCoroutine(callable $test): void
     {
+        if (!class_exists(\Swoole\Coroutine::class)) {
+            self::markTestSkipped('Swoole Coroutine is required for DNS tests.');
+        }
+
         \Swoole\Coroutine\run($test);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/Network/Validators/DNSTest.php` around lines 11 - 14, The private
helper runInCoroutine currently calls \Swoole\Coroutine::run() unguarded; update
runInCoroutine to first check class_exists(\Swoole\Coroutine::class) and call
$this->markTestSkipped('Swoole Coroutine not available') when false, otherwise
proceed to \Swoole\Coroutine::run($test); ensure you modify the runInCoroutine
method in the test class (the method named runInCoroutine) to implement this
skip-guard pattern used elsewhere.