Allow usage with PSR-7 v2

[weierophinney]

Q	A
Documentation	no
Bugfix	no
BC Break	yes
New Feature	yes
RFC	no
QA	yes

Description

This patch adapts Diactoros to work with either PSR-7 v1.1 or v2.0 by explicitly adding the parameter type hints defined in PSR-7 v2.0, as well as void return type declarations where not previously declared.

This patch depends on the following upstream changes:

• Support psr/http-message v2 php-fig/http-factory#13 (will likely be tagged 1.1)
• Adapt tests to work with v1.1 and v2.0 of PSR-7 php-http/psr7-integration-tests#68 (will likely be tagged 1.3)

This patch should be applied to an upcoming 3.0.x branch, for release as 3.0, due to the following BC breaks:

• Addition of explicit parameter type declarations (where known) to all PSR-7 implementation signatures. This change will break any extensions of these classes, as the signatures have changed in a way that does not conform to covariance rules for return types. Specifically, any method that was updated to return void in the PSR-7 spec that we had not marked in our own implementation with a void return declaration leads to the BC break, as updating our implementation means extensions that do not declare it will now cause a fatal error.

Tasks

The following are required before removing this from draft status:

• Merge and release of PSR-7 integration test suite changes
• Merge and release of PSR-17 updates
• Removal of temporary repositories used to test against the above two dependencies

[TimWolla]

This change will break any extensions of these classes, as the signatures have changed in a way that does not conform to contravariance rules for parameters.

I do not believe this description is accurate. A deriving class may leave out parameter types of a parent class (“widen the type”), because everything that may be passed to the parent class may still be passed to the deriving class: https://3v4l.org/gfWBN

However adding parameter types may break consumers that rely on passing an invalid type. However to my understanding, Diactoros already included checks to validate the type.

So, I believe this is safe for a minor, unless I'm missing something?

[weierophinney]

@TimWolla —

So, I believe this is safe for a minor, unless I'm missing something?

You were right about the contravariance versus covariance bit; I had that part wrong in the description and have since corrected it.

The problem is actually with the return type hints: when we were defining return type hints in this implementation, we missed a few cases that later became void in the PSR-7 v2 spec. Going from implied mixed to void is the BC break. If an extension of a method now defined to return void returns something or omits the return type declaration, PHP will raise a fatal error.

[boesing]

I wonder if we should pin php-http integration tests to specific versions anyway. Since they can change upstream, it might lead to failing checks in PRs which are unrelated to those changes. Having renovate bumping these commits could help, no?

[Xerkus]

Having renovate bumping these commits could help, no?

That would only affect latest branch.
Integration tests could be updated to cover newly discovered cross-compatibility issue and we would want CI fail for older branches too, imo.

[boesing]

That would only affect latest branch.

More than enough for PRs.

Integration tests could be updated to cover newly discovered cross-compatibility issue and we would want CI fail for older branches too, imo.

Having scheduled build for that where we explictly use composer require integration-tests:dev-master should be used for discovering these kinds of issues.

[Xerkus]

Both dependencies got releases. 1.0.2 and 1.3.0 respectively

[weierophinney]

I've now created the 3.0.x branch, and updated this patch to be against that branch. Additionally, both psr/http-factory and php-http/psr7-integration-tests have released stable versions that support PSR-7 1.1 and 2.0, allowing me to rebase my patch to reference the stable branches.

Ready for final review and release! Once done, I'll turn my efforts to laminas/laminas-stratigility, and then the various Mezzio components.

[boesing]

LGTM

[Ocramius]

🚢

Thanks @weierophinney!

[Xerkus]

laminas-diactoros/src/RequestTrait.php

Line 156 in 22eca5b

public function withRequestTarget($requestTarget): RequestInterface

Missing parent type declaration.

---

Some tangentially related comments:

laminas-diactoros/src/UploadedFile.php

Lines 60 to 62 in 22eca5b

	public function __construct(
	$streamOrFile,
	private int $size,

Interface defines size as nullable. This should probably be changed to nullable too. If so this will need to be backported to 2.x

laminas-diactoros/src/MessageTrait.php

Line 353 in 22eca5b

private function validateProtocolVersion($version): void

MessageTrait::validateProtocolVersion() could have argument type defined and is_string check dropped to match withProtocolVersion().

laminas-diactoros/src/ServerRequest.php

Line 176 in 22eca5b

public function getAttribute(string $attribute, $default = null)

This uses parameter name $attribute and interface uses $name. Should be changed for consistency. Same for withAttribute()

laminas-diactoros/src/StreamFactory.php

Line 31 in 22eca5b

public function createStreamFromFile(string $file, string $mode = 'r'): StreamInterface

Interface uses $filename

[weierophinney]

Missing parent type declaration.

Thanks - updating that now.

[weierophinney]

Interface defines size as nullable. This should probably be changed to nullable too. If so this will need to be backported to 2.x

While the size can be nullable, we're saying in this particular implementation that it must be an integer. Since this narrows what we return from getSize(), it's still valid and fulfills the spec. I see no reason to change this currently.

[Xerkus]

While the size can be nullable, we're saying in this particular implementation that it must be an integer.

This somewhat conflicts with the argument of UriFactory as using null will lead to type error

laminas-diactoros/src/UploadedFileFactory.php

Lines 18 to 30 in 22eca5b

	public function createUploadedFile(
	StreamInterface $stream,
	?int $size = null,
	int $error = UPLOAD_ERR_OK,
	?string $clientFilename = null,
	?string $clientMediaType = null
	): UploadedFileInterface {
	if ($size === null) {
	$size = $stream->getSize();
	}
	return new UploadedFile($stream, $size, $error, $clientFilename, $clientMediaType);
	}

[weierophinney]

laminas-diactoros/src/ServerRequest.php

Line 176 in 22eca5b

public function getAttribute(string $attribute, $default = null)

This uses parameter name $attribute and interface uses $name. Should be changed for consistency. Same for withAttribute()

laminas-diactoros/src/StreamFactory.php

Line 31 in 22eca5b

public function createStreamFromFile(string $file, string $mode = 'r'): StreamInterface

Interface uses $filename

I think these should be done with the 2.x series, and then merged up.

[weierophinney]

While the size can be nullable, we're saying in this particular implementation that it must be an integer.

This somewhat conflicts with the argument of UriFactory as using null will lead to type error

Alright, that argument I can get behind. I'll make it nullable, but not have a default value.

[Xerkus]

Alright, that argument I can get behind. I'll make it nullable, but not have a default value.

Will you make PR against 2.x or should I?

[weierophinney]

@Xerkus —

I've pushed changes for each of the following issues you flagged:

• Added the missing string parameter type declaration to RequestTrait::withRequestTarget()
• Added a string parameter type declaration to MessageTrait::validateProtocolVersion()'s $version argument, and removed the unneeded conditional that checked for a string in that method.
• Made the $size argument/property in UploadedFileFactoryInterface nullable.

I've been thinking about the parameter name changes, and it DOES NOT make sense to do them on the 2.x branch, as, with named arguments in PHP 8, this would be a BC break (name changes). While it could be considered a bugfix, it would potentially break code for users. So let's do it here; I'll push those shortly.

[Xerkus]

@weierophinney thank you!