Skip to content

Simplify error handling for dns_get_record() #378

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

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from
Open

Simplify error handling for dns_get_record() #378

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 2 additions & 14 deletions src/Validation/DNSGetRecordWrapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,7 @@ class DNSGetRecordWrapper
*/
public function getRecords(string $host, int $type): DNSRecords
{
// A workaround to fix https://bugs.php.net/bug.php?id=73149
set_error_handler(
static function (int $errorLevel, string $errorMessage): never {
throw new \RuntimeException("Unable to get DNS record for the host: $errorMessage");
}
);
try {
// Get all MX, A and AAAA DNS records for host
return new DNSRecords(dns_get_record($host, $type));
} catch (\RuntimeException $exception) {
return new DNSRecords([], true);
} finally {
restore_error_handler();
}
$result = @dns_get_record($host, $type);
Copy link
Owner

Choose a reason for hiding this comment

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

So I understand set_error_handler is possibly interfering with the broader project that might itself be setting that same error handler. Now, the function is releasing / restoring the handler at the end.
The aim is to catch these errors.
Suppressing them defeats the purpose. So, what do you think if we find a way for this to be optional with the suppressed option being the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I understand set_error_handler is possibly interfering with the broader project that might itself be setting that same error handler.

Not really. In the set_error_handler function we throw an exception which we catch directly in the same file. The old code with comments.

        set_error_handler(
            static function (int $errorLevel, string $errorMessage): never {
                // Here we throw an exception if dns_get_record function failed.
                throw new \RuntimeException("Unable to get DNS record for the host: $errorMessage");
            }
        );
        try {
            // Get all MX, A and AAAA DNS records for host
            return new DNSRecords(dns_get_record($host, $type));
        } catch (\RuntimeException $exception) {
            return new DNSRecords([], true); // And here we catch the exception thrown in set_error_handler.
        } finally {
            restore_error_handler();
        }

Therefore, my changes do not introduce any changes in logic - just a shorter entry.

Copy link
Contributor Author

@aivchen aivchen Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I didn't see that we pass true param in the catch block. Let me fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Owner

Choose a reason for hiding this comment

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

I reviewed the PHP bug (https://bugs.php.net/bug.php?id=73149 ) being addressed by the workaround and it is still happening.
In short, the bug allows an unexpected error to bypass the error suppression (@) which being un-handled then breaks the execution not only of the library but of the system using it. Which would then require our users to wrap, just in case, the call in a try/catch statement.
That's something I try to avoid the users of the library to do.

So we cannot simplify the code unless the bug is fixed or a different approach is found.

return new DNSRecords($result === false ? [] : $result, $result === false);
}
}