Skip to content

Resolver::resolveAll - #67

Closed
WyriHaximus wants to merge 3 commits into
reactphp:masterfrom
WyriHaximus-labs:resolve-all
Closed

Resolver::resolveAll#67
WyriHaximus wants to merge 3 commits into
reactphp:masterfrom
WyriHaximus-labs:resolve-all

Conversation

@WyriHaximus

Copy link
Copy Markdown
Member

The status quo with the resolver is that resolve will always return one IP address when resolving a hostname, even when the query response yields more. This proposed resolveAll method will return all addresses yielded from the query response.

$factory = new Factory();
$resolver = $factory->create('8.8.8.8', $loop);

$name = isset($argv[1]) ? $argv[1] : 'blog.wyrihaximus.net';

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

In case anyone wonders why my own blog, because it is hosted on AWS CloudFront and this always returns more then one IP Address

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe add that as a code comment rather than review comment.

});
}

public function extractAddress(Query $query, Message $response)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this an intended BC break?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree here, while this method isn't intended to be used from the outside, it's still part of our (current) public API.

@clueclue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, let's resolve the minor BC break below and let's get this in! :shipit:

});
}

public function extractAddress(Query $query, Message $response)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree here, while this method isn't intended to be used from the outside, it's still part of our (current) public API.

@WyriHaximusWyriHaximus added this to the v0.4.11 milestone Aug 23, 2017
@WyriHaximus

Copy link
Copy Markdown
MemberAuthor

Ping @clue@jsor

@clueclue removed this from the v0.4.11 milestone Aug 23, 2017
@clueclue added this to the v0.4.16 milestone Jun 28, 2018
@clueclue removed this from the v0.4.16 milestone Jun 29, 2018
@jsorjsor closed this in #110 Jun 29, 2018
Sign up for freeto 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.

3 participants

@WyriHaximus@clue@kelunik