Skip to content
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.

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

support AAAA #58

Merged
merged 1 commit into from Apr 15, 2017
Merged

support AAAA #58

merged 1 commit into from Apr 15, 2017

Conversation

othillo
Copy link
Contributor

@othillo othillo commented Apr 10, 2017

Hi, this PR adds the AAAA record type to react/dns.

@cboden
Copy link
Member

cboden commented Apr 10, 2017

Awesome!

@WyriHaximus
Copy link
Member

IMHO we should add this 👍

@jsor jsor added this to the v0.4.8 milestone Apr 13, 2017
@clue
Copy link
Member

clue commented Apr 13, 2017

Awesome! Functionally LGTM – But the goal is a bit unclear from an integrators view point, given that I don't see this being used anywhere? How is (can) this used or is this only an internal preparation? Also, this only handles parsing incoming responses, but we never actually prepare any such outgoing requests afaict?

(Also applies to #59 afaict)

@asm89
Copy link

asm89 commented Apr 14, 2017

@clue We use an Executor directly. E.g. $executor->query($server, new Query(...));. Same for the PTR PR.

@clue
Copy link
Member

clue commented Apr 14, 2017

@asm89 This is interesting! I don't see this covered anywhere in either the README, examples or functional tests, does it make sense to you to add functional/integration tests as part of this PR? 👍

@asm89
Copy link

asm89 commented Apr 15, 2017

@asm89 This is interesting! I don't see this covered anywhere in either the README, examples or functional tests

It's covered in the ExecutorInterface and it's how the Resolver uses the executor. Using the Executor directly does seem a bit like using an internal class at the moment. If you and other maintainers agree it would be great to see the interface to get advertised for more advanced use cases. The Resolver is quite limited at the moment. When resolving A records we for example want to get all of them. This is only possible by bypassing the Resolver.

does it make sense to you to add functional/integration tests as part of this PR?

I'd rather see it being done in a follow up. The parser code is unit tested. We do use the executor directly now and it seems to work fine. The library does not have any integration tests for the Executor it seems, only some unit tests that mock out the interactions with the Parser. That doesn't seem like to big of a deal as the executor takes care of the data connection part. The parser does the heavy lifting of parsing the response. Whatever the parser returns is directly returned through the API of the executor:

dns/src/Query/Executor.php

Lines 113 to 132 in e09ca28

$response = $parser->parseMessage($data);
} catch (\Exception $e) {
$conn->end();
$deferred->reject($e);
return;
}
if ($response->header->isTruncated()) {
if ('tcp' === $transport) {
$deferred->reject(new BadServerException('The server set the truncated bit although we issued a TCP request'));
} else {
$conn->end();
$deferred->resolve($retryWithTcp());
}
return;
}
$conn->end();
$deferred->resolve($response);
.

@WyriHaximus
Copy link
Member

If you and other maintainers agree it would be great to see the interface to get advertised for more advanced use cases.

👍 from me on that. Although that would be better suited in a different PR.

does it make sense to you to add functional/integration tests as part of this PR?

I'd rather see it being done in a follow up.

Agreed, this fits perfectly in the way of small PR's we've been doing lately

@clue
Copy link
Member

clue commented Apr 15, 2017

@asm89 Thanks for the background info! I was mostly curious as to how we should describe (sell) this feature in the README/CHANGELOG and I agree that it makes sense to get this in as-is 👍 A follow-up PR to document and test this "advanced usage" would be much appreciated 👍

Thanks for the high quality PR, LGTM! :shipit:

@WyriHaximus WyriHaximus merged commit 9c8225e into reactphp:master Apr 15, 2017
@othillo
Copy link
Contributor Author

othillo commented Apr 15, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants