Conversation
|
Prelim remark: how about adding that code example (and maybe some more text) to the documentation for the website ? As in: either add or update one of the existing markdown files in the |
|
@schlessera Would you mind also having a look at what's happening with the code coverage ? I would not expect a new feature, which is described as "fully tested" to reduce code coverage. |
jrfnl
left a comment
There was a problem hiding this comment.
This is not a complete review (yet). Posting it anyway to allow for further iteration on this PR based on my remarks.
| /** | ||
| * Requests for PHP, an HTTP library. | ||
| * | ||
| * @copyright 2012-2023 Requests Contributors |
There was a problem hiding this comment.
| * @copyright 2012-2023 Requests Contributors | |
| * @copyright 2012-2026 Requests Contributors |
(here and elsewhere - or should we do this codebase wide after this PR has been merged ?)
| * Exception for a missing IP address in the host bindings. | ||
| * | ||
| * @package Requests\Exceptions | ||
| * @since 2.x.x |
There was a problem hiding this comment.
| * @since 2.x.x | |
| * @since 2.1.0 |
| * Instantiate a MissingIpAddress exception for a missing IP address in the host bindings. | ||
| * | ||
| * @param string $host Host that was requested. | ||
| * @return self |
There was a problem hiding this comment.
| * @return self | |
| * @return \WpOrg\Requests\Exception\MissingIpAddress |
| * requested via HostBindings. | ||
| * | ||
| * @param string $host Unknown host that was requested. | ||
| * @return self |
There was a problem hiding this comment.
| * @return self | |
| * @return \WpOrg\Requests\Exception\UnknownHost |
| * Exception for an unknown host being requested via HostBindings. | ||
| * | ||
| * @package Requests\Exceptions | ||
| * @since 2.x.x |
There was a problem hiding this comment.
| * @since 2.x.x | |
| * @since 2.1.0 |
| * @throws UnknownHost If the requested host does not have a mapping. | ||
| * @throws MissingIpAddress If the requested host has no IP address available. |
There was a problem hiding this comment.
| * @throws UnknownHost If the requested host does not have a mapping. | |
| * @throws MissingIpAddress If the requested host has no IP address available. | |
| * | |
| * @throws \WpOrg\Requests\Exception\UnknownHost If the requested host does not have a mapping. | |
| * @throws \WpOrg\Requests\Exception\MissingIpAddress If the requested host has no IP address available. |
| * This throws an exception if the requested host does not have a mapping. | ||
| * This also throws an exception if the requested host has no IP addresses available. |
There was a problem hiding this comment.
This looks redundant, what with the @throws tags containing the same info.
| * | ||
| * @param string $host Host to request a mapping for. | ||
| * @return array<string> IP address mappings for the host. | ||
| * @throws UnknownHost If the requested host does not have a mapping. |
There was a problem hiding this comment.
| * @throws UnknownHost If the requested host does not have a mapping. | |
| * | |
| * @throws \WpOrg\Requests\Exception\UnknownHost If the requested host does not have a mapping. |
| * This throws an exception if the requested host does not have a mapping. | ||
| * Contrary to get_first_ip_for_host(), this method does not throw an exception | ||
| * if the host has no IP addresses available. It will simply return an empty array. |
There was a problem hiding this comment.
Just wondering - why still throw an exception if the host doesn't have a mapping ? Why not return an empty array in that case too ?
src/Transport/Curl.php
Outdated
| // Fallback branches for cURL < 7.49.0 which lacks CURLOPT_CONNECT_TO. | ||
| // Cannot be reached in tests as CURLOPT_CONNECT_TO is always defined. | ||
| // @codeCoverageIgnoreStart -- The else is never reached in CI because CURLOPT_CONNECT_TO is always defined. |
There was a problem hiding this comment.
| // Fallback branches for cURL < 7.49.0 which lacks CURLOPT_CONNECT_TO. | |
| // Cannot be reached in tests as CURLOPT_CONNECT_TO is always defined. | |
| // @codeCoverageIgnoreStart -- The else is never reached in CI because CURLOPT_CONNECT_TO is always defined. | |
| // Fallback branches for cURL < 7.49.0 which lacks CURLOPT_CONNECT_TO. | |
| // Cannot be reached in tests as CURLOPT_CONNECT_TO is always defined. | |
| // The `else` is never reached in CI because CURLOPT_CONNECT_TO is always defined. | |
| // @codeCoverageIgnoreStart |
Might need testing, but I have a niggly feeling that the PHPUnit code coverage annotations do not support adding a comment behind the annotation.
That might explain why the comment isn't working as intended when viewing the CodeCov report.
Pull Request Type
This is a:
Context
This PR adds support for binding specific hostnames to specific IP addresses, bypassing DNS resolution.
This is useful for:
HostheaderDetailed Description
New
HostBindingsutility classA value object that validates and stores hostname-to-IP mappings. Key features:
filter_var(FILTER_VALIDATE_IP)to prevent hostname injection attacks. Accepts both IPv4 and IPv6 addresses (including private/localhost ranges).HostBindings::SKIP_IP_VALIDATION(use with caution).Methods:
has_host($host)- Check if a host has a mappingget_first_ip_for_host($host)- Get the first IP for a host (throwsUnknownHostorMissingIpAddressif not available)get_all_ips_for_host($host)- Get all IPs for a host (throwsUnknownHostif not found)New exceptions
UnknownHost- Thrown when requesting a host that isn't in the bindingsMissingIpAddress- Thrown when a host has no IP addresses configuredNew
HOST_BINDINGScapabilityAdded to the
Capabilityinterface to allow transport capability testing.Transport integration
Curl transport (
Transport\Curl):CURLOPT_CONNECT_TO(cURL 7.49.0+) as the preferred methodCURLOPT_RESOLVE(cURL 7.21.3+) for older cURL versionsHostheaderFsockopen transport (
Transport\Fsockopen):HostheaderUsage
The
host_bindingsoption accepts either an array (with automatic IP validation) or a pre-constructedHostBindingsobject:Includes
HostBindingsclass (constructor validation, all methods)Transport\BaseTestCasecovering both transportsRequests::request()docblock