Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #30 by improving error handling for DNS response parsing failures. The changes introduce a new exception type for invalid responses and add safeguards against unpack() failures when parsing DNS answer headers.
Key Changes:
- Added
InvalidResponseexception class for DNS response parsing errors - Implemented error handling for empty/null array results from
unpack()with optional exception throwing - Suppressed warnings from
unpack()using the@operator
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/PurplePixie/PhpDns/Exceptions/InvalidResponse.php | New exception class for handling invalid DNS response errors |
| src/PurplePixie/PhpDns/DNSQuery.php | Added response exception flag, error handling for unpack failures, and getter/setter methods for the new flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ' TTL ' . $ans_header['ttl'] . ' Length ' . $ans_header['length'] | ||
| ); | ||
| else $this->debug("Error unpacking answer header, no array returned."); | ||
| if ($ans_header === null || $ans_header === false || !is_array($ans_header)) // the unpack has failed - we assume an invalid return |
There was a problem hiding this comment.
The condition $ans_header === null is redundant. The unpack() function in PHP only returns false on failure, never null. The check should be simplified to if ($ans_header === false).
| if ($ans_header === null || $ans_header === false || !is_array($ans_header)) // the unpack has failed - we assume an invalid return | |
| if ($ans_header === false) // the unpack has failed - we assume an invalid return |
| else | ||
| { | ||
| $this->debug( | ||
| 'Record Type ' . $ans_header['type'] . ' Class ' . $ans_header['class'] . | ||
| ' TTL ' . $ans_header['ttl'] . ' Length ' . $ans_header['length'] | ||
| ); | ||
| } |
There was a problem hiding this comment.
The else block is unnecessary since the if block returns an array. The code in the else block (lines 224-227) should be moved after the if block and the else removed to reduce nesting and improve readability.
| $ans_header_bin = $this->readResponse(10); // 10 byte header | ||
|
|
||
| $ans_header = unpack('ntype/nclass/Nttl/nlength', $ans_header_bin); | ||
| $ans_header = @unpack('ntype/nclass/Nttl/nlength', $ans_header_bin); |
There was a problem hiding this comment.
Using the @ operator to suppress errors is generally discouraged as it hides potential issues and makes debugging harder. Consider removing the @ operator and handling the false return value explicitly, which you're already doing in the subsequent if statement.
| $ans_header = @unpack('ntype/nclass/Nttl/nlength', $ans_header_bin); | |
| $ans_header = unpack('ntype/nclass/Nttl/nlength', $ans_header_bin); |
Resolve issue #30 - added error handling for empty array and optional exception, also suppress warnings from
unpack()