hhvm/hack-http-request-response-interfaces

Defines common cross-framework interfaces to represent HTTP requests and responses

hack-http-request-response-interfaces's Issues

URIInterface: type mismatch between getUserInfo() and withUserInfo()

• withUserInfo() takes string $user, ?string $password
• getUserInfo() always returns string

Options include:

• making getUserInfo() return ?(string, ?string)
• making getUserInfo() return ?shape('user' => string, 'pass' => ?string)
• separate getters for user and password (inconsistent with everything else, but nice for typing)

URIInterface: nullable strings vs empty strings?

Currently, most accessors are defined as returning empty strings - however, this is mildly inconsistent with getPort() and withPort(), which deal with ?int.

URIInterface: withQuery requires an encoded string

It could be safer to rename it to withRawQuery (+ getRawQuery()), and make withQuery take dict<string, ?string> and require implementations to encode/escape, instead of application developers.

RequestInterface: withUri() preserveHost parameter

withUri takes a new UriInterface and a bool preserveHost parameter. We generally consider bool parameters to be a code smell, as they harm readability.

Options include:

• splitting it to separate methods
• replacing it with an options shape
• always taking the provided URI verbatim and entirely replacing the previous URI

MessageInterface: withHeader and withAddedHeader() are not properly typed

They currently accept either a single value or an array of values. Either we should only allow one case, or we should split it into two.

Replace UploadedFileInterface::getError() value with an enum

Currently defiend in terms of $_FILES and PHP's UPLOAD_ERR_XXX constants

ServerRequestInterface::getServerParams() are mixed

For example:

• GATEWAY_INTERFACE is a string
• argv is an array

Options include:

• making it a shape
• splitting it into many methods
• make it return a new ServerParams object with many methods

Remove __toString() from StreamInterface and URIInterface?

Support for implicit coercions seems unnecessary.

Re-evalualate nullable stuff in URIs

cc @usox

I don't think we should return ?string where there is not an observerable difference between not provided and empty when using HTTP, or in the string representation of URIs

For example, getRawQuery() - does "/foo" have a different rawquery to "/foo?"? Is the answer the same if we're looking at the current URI and it was a request to one of those?

RequestInterface: getRequestTarget() and withRequestTarget() are untyped

• getRequestTarget() is documented as returning a string, however this isn't the case if withRequestTarget() is called, which explicitly allows an implementation-defined object.

Options include:

• make it a generic
• only allow strings; implementations can add an additional method that isn't in the interface if they wish

[Meta] HTTP interfaces rework.

This is a meta issue to track the rework that should be done for these interfaces and the problems they have;

ServerRequestInterface::getAttributes() and related functions have mixed values

Options include:

• make it a generic shape
• remove entirely: implementations can provide more specific accessors without them being in the implementation

Dependency error via Composer

I am running HHVM version 3.29

Problem 1
    - The requested package facebook/hack-http-request-response-interfaces ^0.1 exists as facebook/hack-http-request-response-interfaces[dev-master] but these are rejected by your constraint.

Composer package not found in ^0.1.

Packagist / Latest Version b83a106
Github / Latest Version 5baf46d
does not match git commit hashes

StreamInterface::getMetadata() is untyped

The type depends on whether a key was provided. Options include:

• never taking a key
• splitting it to two functions - one returning a dict, one returning mixed

This matters more for Hack as Hack dissallows indexing mixed values.

Explicit support for Set-Cookie header

Not sure that this actually should be here, but we should put some more thought into it.

We could provide basically an equivalent of the setcookie() function, setting extra parameters. This is what I expected to see.

However, perhaps we should leave it to implementations to provide (on top of withHeaderLine) in the way they see fit.

ServerRequestInterface: getCookieParams() is defined in terms of $_COOKIE

Should replace it with a more specific encoding - e.g. 'dots replaced with underscores''

Use shared github action instead of Travis

ServerRequestInterface: getQueryParams() and getUploadedFiles() are untyped

They're untyped as they return an arbitrarily nested tree.

Options include:

• returning a dict<string, Node> where Node may be a Leaf or another object containing dict<string, Node>
• flattening the tree

Abstract away `resource` (streams)

resources are not type-safe and are a PHPIsm. Some options are:

1. add support for directly using strings
2. omit support for now
3. use HH\Lib\Experimental\IO\Handle (https://github.com/hhvm/hsl-experimental)

My instinct would be both (1) and (3)

change ServerRequestInterface query parameters to return dict<string, vec<string>>

UriInterface comment return type and actual return type are inconsistent.

Several functions' comments in UriInterface state that the return type is null|string, but the declared return type in the signature is a non-nullable string. Are the comments wrong, or should the functions return a nullable string?

Examples:

hack-http-request-response-interfaces/src/UriInterface.hack

Lines 64 to 66 in ab9e095

	* @return null|string The URI scheme.
	*/
	public function getScheme(): string;

hack-http-request-response-interfaces/src/UriInterface.hack

Lines 83 to 85 in ab9e095

	* @return null|string The URI authority, in "[user-info@]host[:port]" format.
	*/
	public function getAuthority(): string;

hack-http-request-response-interfaces/src/UriInterface.hack

Lines 106 to 108 in ab9e095

	* @return null|string The URI host.
	*/
	public function getHost(): string;

hack-http-request-response-interfaces/src/UriInterface.hack

Lines 172 to 174 in ab9e095

	* @return null|string The percent-encoded query string
	*/
	public function getRawQuery(): string;

RequestInterface: getMethod() and withMethod() could take enums for HTTP methods

StreamInterface::read should return string

Define what happens when key-like values are 'case insensitive'

refs #27

For example, if I call ->withHeader('foo', vec['bar']) and ->withHeader('Foo', vec['baz']), what exactly goes down the wire?

• definitely not bar
• do we normalize keys, or does last win?
• or explicitly implementaiton defined?

Change server request query params, cookie params, and parsed body to return vec<(string, string)>

closes #36

ServerRequestInterface::getParsedBody() is untyped

There's lot of explicitly implementation-defined behavior here. I don't think there's a good option other than to remove it, and add a new function explicitly for application/x-www-form-urlencoded data that always returns dict<string, string>, or null/throw an exception if not form-urlencoded

Implementation-specific behavior can be added to concrete classes without defining it in the interface.