Merged

Conversation

jsor approved these changes Apr 12, 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:

	$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);

clue approved these changes Apr 15, 2017