Adapt tests to work with v1.1 and v2.0 of PSR-7 #68

weierophinney · 2023-04-04T20:02:34Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	Provides more comprehensive change than #65. Fix #64
Documentation	TBD
License	MIT

What's in this PR?

Refactors tests that were using the assumption of throwing an InvalidArgumentException to test for either InvalidArgumentException OR TypeError.

The change goes further than #65 in that it provides test changes, not just dependency changes. The test changes have been vetted by running against a laminas-diactoros checkout that modifies its codebase to conform to the PSR-7 changes.

Why?

This change will allow testing against PSR-7 1.1+ releases safely.

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

weierophinney · 2023-04-04T21:16:54Z

@dbu So... I have a bit of a conundrum.

A number of the tests, when run against an implementation, will result in failures UNLESS I modify the test cases you provide to add declare(strict_types=1). The reason is because when not used with strict mode, the values will be munged to fit the declared type, allowing them to be used "safely". As a result, the test cases now result in the AssertionFailedError I raise if the methods do not otherwise raise a validation error.

If I update to enable strict types, they work correctly.

The tests I specifically ran up against were:

RequestIntegrationTest::testMethodWithInvalidArguments()
ResponseIntegrationTest::withStatusCodeInvalidArgument()

In Laminas, we always declare strict types, and do so in our tests as well, so I'd not come across this. However, because the base abstract test cases defined here do not.... now we're hitting the issue.

I was rather excited about being able to rip out validation code that was unnecessary due to having scalar parameter type declarations, but if we cannot assume strict types are enabled, it seems they need to be there... and that then raises errors with static analysis due to redundant checks. 😠

How would you like to proceed? Should I add the strict types declaration as part of ths patch?

simPod

Dataprovider for Tests\Nyholm\Psr7\Integration\ResponseTest::testStatusCodeInvalidArgument needs to have inputs of invalid types removed.

(keep only ints for strict types or remove only the float one)

also Tests\Nyholm\Psr7\Integration\UriTest::testWithSchemeInvalidArguments (true and 34)

weierophinney · 2023-04-05T20:12:14Z

I've updated the PR to take the advice of @simPod , and removed the provider values that fail when strict types is not enabled.

simPod

also Tests\Nyholm\Psr7\Integration\UriTest::testWithSchemeInvalidArguments (true and 34)

this is missing yet

weierophinney · 2023-04-06T14:15:16Z

@simPod —

also Tests\Nyholm\Psr7\Integration\UriTest::testWithSchemeInvalidArguments (true and 34)

this is missing yet

Those should stay. The spec cleary states:

Implementations MUST support the schemes "http" and "https" case insensitively, and MAY accommodate other schemes if required.

If an implementation is allowing casting of true or 34 to strings ("1" and "34" respectively) and considering them valid, then they are incorrectly allowing invalid schemes. RFC-3986 section 3.1 requires a scheme to start with an alphabetical character. As such, regardless of whether or not strict types is enabled, those values MUST be considered invalid.

simPod · 2023-04-06T14:54:46Z

But mind this test https://github.com/Nyholm/psr7/blob/c06c9e0f03ca36a2c4e1823c34a8937eec90ed11/tests/UriTest.php#L204 It seems schema values should not be validated based on that.

weierophinney · 2023-04-06T16:47:39Z

But mind this test https://github.com/Nyholm/psr7/blob/c06c9e0f03ca36a2c4e1823c34a8937eec90ed11/tests/UriTest.php#L204 It seems schema values should not be validated based on that.

That's a problem in that particular implementation; that scheme value should not be accepted.

simPod · 2023-04-06T16:49:37Z

I have added a validation and removed that test case, will see how it goes in review in Nyholm/psr7#223

dbu · 2023-04-07T08:36:23Z

thanks for the merge request!

How would you like to proceed? Should I add the strict types declaration as part of ths patch?

i would be ok to enable strict typing in the test cases, it is best practice to have them enabled.

is there a way to discover the version of the PSR interfaces that is used? then we could $this->markAsSkipped() the test cases that become obsolete with strict typing. that way we would still test correct behaviour of 1.0 implementations that lack the typing but not have redundant tests that the type declarations already enforce.

weierophinney · 2023-04-07T18:34:43Z

is there a way to discover the version of the PSR interfaces that is used? then we could $this->markAsSkipped() the test cases that become obsolete with strict typing. that way we would still test correct behaviour of 1.0 implementations that lack the typing but not have redundant tests that the type declarations already enforce.

Yes; Composer provides this. That said, I don't think it's necessary. By testing if either InvalidArgument or TypeError are thrown for the given arguments, we are able to test against either version of the spec. The only reason I needed to remove any provider cases was due to strict mode not being present in these test class files; if that's added, then we can keep these, and test against all versions without issue.

I'm happy to update to do this in this patch.

dbu · 2023-04-10T07:35:18Z

strict mode not being present in these test class files; if that's added, then we can keep these, and test against all versions without issue.

then lets do it that way.

i would assume that if a sloppy conversion is possible, sloppy code would end up with working requests. so people who do not use strict will be happy, and those that use strict will also be happy.

Nyholm

Im happy with this PR

dbu · 2023-04-17T07:01:03Z

can you please rebase on master? this will make this use the github actions so we can see the builds (there will be failures for some clients, we found no way to mark them as allowed to fail but will still merge if there is not mistake with the tests)

Refactors tests that were using the assumption of throwing an `InvalidArgumentException` to test for either `InvalidArgumentException` OR `TypeError`. This change will allow testing against PSR-7 1.1+ releases safely.

When performing try/catch blocks, and the catch represents a valid condition, we need to assert SOMETHING, or else the test is flagged as risky. A simple assertion that we got a throwable works in these cases.

Adds a `fail()` call when an invalid argument is not caught.

@simPod

Per a comment from @simPod, removes the following: - From `RequestIntegrationTest::getInvalidMethod()`: - The test cases for 1 and 1.01 (integer and float) - From `ResponseIntegrationTest::getInvalidStatusCodeArguments()`: - The test cases for 200.34 Additionally, this patch adds keys to each entry returned by these two providers, to better facilitate understanding which test failed. The removals were necessary as the test case does not enable strict mode, and these particular types can be converted to the specified type "safely", leading to false negative results.

Evidently, aligning operators is not allowed in this project. (I wish the required style guide was published with the project...)

weierophinney · 2023-04-17T20:17:28Z

@dbu Rebased!

weierophinney · 2023-04-18T19:56:51Z

@dbu What are the next steps? This is the last library that Diactoros needs updated before we can release PSR-7 v2 support. We are currently testing against this PR successfull (see laminas/laminas-diactoros#136), but are waiting to release until we can update to test against an official release.

For others who are writing implementations: are any of you having issues with this patch? If so, I'd be happy to review and see if either (a) it requires updates/fixes in your library , or (b) the patch needs revisions to better mirror the specification interfaces.

dbu · 2023-04-19T07:03:17Z

the CI shows that i made a mistake in the CI script: we install the dependencies and then require the implementation without allowing to update dependencies.

can you delete the line that does composer update before doing the composer require? that would be more efficient anyways and would run the builds again. in master the laminas builds for PHP 8 are green :-)

@dbu

Per @dbu

weierophinney · 2023-04-19T19:18:29Z

can you delete the line that does composer update before doing the composer require?

Done!

One thing to note: current Diactoros offerings are only on PHP 8.0 and above, so the tests for PHP 7 versions will ALWAYS fail here. Is it possible to change the matrix so it doesn't try testing those?

dbu · 2023-04-20T06:34:26Z

thanks a lot! i merged as it updates the current state to know about PSR-7 1.1 and 2.0

current Diactoros offerings are only on PHP 8.0 and above, so the tests for PHP 7 versions will ALWAYS fail here. Is it possible to change the matrix so it doesn't try testing those?

i noticed when i set up the build matrix and was also wondering. there might be some value in knowing what the problems of outdated versions of diactoros are. the most comprehensive would be to split off some sort of legacy test suite for the no longer maintained versions and update the table to show the status of the maintained versions and also specify what PHP version should be used.

weierophinney · 2023-04-21T17:42:49Z

thanks a lot! i merged as it updates the current state to know about PSR-7 1.1 and 2.0

My pleasure!

Question: when will this be included in a release, and what version (I'm assuming 1.3.0)? Trying to plan when I can complete the tasks for Diactoros. 😄

This is until a 1.3.0 release is made, and represents the commit in which the changes in php-http/psr7-integration-tests#68 were merged. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>

dbu · 2023-04-28T14:32:09Z

i was on holidays this week and managed to not open my laptop for several days :-D

i now split the tests for the badges in the README and laminas is looking good 👍

i just tagged https://github.com/php-http/psr7-integration-tests/releases/tag/1.3.0

weierophinney added a commit to weierophinney/psr7-integration-tests that referenced this pull request Apr 4, 2023

docs: provides a CHANGELOG entry for php-http#68

c8feb97

weierophinney mentioned this pull request Apr 4, 2023

Allow usage with PSR-7 v1.1 laminas/laminas-diactoros#133

Merged

dbu mentioned this pull request Apr 5, 2023

Allow psr/http-message v2 #65

Closed

2 tasks

simPod suggested changes Apr 5, 2023

View reviewed changes

weierophinney mentioned this pull request Apr 5, 2023

Allow usage with PSR-7 v2 laminas/laminas-diactoros#136

Merged

3 tasks

simPod suggested changes Apr 6, 2023

View reviewed changes

simPod mentioned this pull request Apr 6, 2023

chore(deps): allow psr/http-message v2 Nyholm/psr7#223

Closed

simPod approved these changes Apr 6, 2023

View reviewed changes

nicolas-grekas mentioned this pull request Apr 14, 2023

Allow psr/http-message v2 Nyholm/psr7#234

Merged

Nyholm approved these changes Apr 16, 2023

View reviewed changes

nyamsprod mentioned this pull request Apr 16, 2023

PSR-7 v2 thephpleague/uri-src#108

Closed

weierophinney added 7 commits April 17, 2023 15:15

feat: update to v2 usage of PSR-7

f8ad2e6

Refactors tests that were using the assumption of throwing an `InvalidArgumentException` to test for either `InvalidArgumentException` OR `TypeError`. This change will allow testing against PSR-7 1.1+ releases safely.

qa: remove extra linebreak

6ed8ec1

docs: provides a CHANGELOG entry for php-http#68

81b8c88

fix: always assert

c45f07d

When performing try/catch blocks, and the catch represents a valid condition, we need to assert SOMETHING, or else the test is flagged as risky. A simple assertion that we got a throwable works in these cases.

fix: ensure invalid arguments not caught by implementation are flagged

545f294

Adds a `fail()` call when an invalid argument is not caught.

qa: adapt code to follow project styles

138b301

Evidently, aligning operators is not allowed in this project. (I wish the required style guide was published with the project...)

weierophinney force-pushed the feature/psr-7-type-hints branch from cb4527d to 138b301 Compare April 17, 2023 20:17

stayallive mentioned this pull request Apr 19, 2023

Fresh Laravel 10 installation needs to downgrade psr/http-message getsentry/sentry-laravel#682

Closed

qa: do not update before require

6608c6a

Per @dbu

dbu merged commit 30a7596 into php-http:master Apr 20, 2023

weierophinney deleted the feature/psr-7-type-hints branch April 21, 2023 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt tests to work with v1.1 and v2.0 of PSR-7 #68

Adapt tests to work with v1.1 and v2.0 of PSR-7 #68

weierophinney commented Apr 4, 2023 •

edited by dbu

Loading

weierophinney commented Apr 4, 2023

simPod left a comment •

edited

Loading

weierophinney commented Apr 5, 2023

simPod left a comment

weierophinney commented Apr 6, 2023 •

edited

Loading

simPod commented Apr 6, 2023

weierophinney commented Apr 6, 2023

simPod commented Apr 6, 2023

dbu commented Apr 7, 2023

weierophinney commented Apr 7, 2023

dbu commented Apr 10, 2023

Nyholm left a comment

dbu commented Apr 17, 2023

weierophinney commented Apr 17, 2023

weierophinney commented Apr 18, 2023

dbu commented Apr 19, 2023

weierophinney commented Apr 19, 2023

dbu commented Apr 20, 2023

weierophinney commented Apr 21, 2023

dbu commented Apr 28, 2023

Adapt tests to work with v1.1 and v2.0 of PSR-7 #68

Adapt tests to work with v1.1 and v2.0 of PSR-7 #68

Conversation

weierophinney commented Apr 4, 2023 • edited by dbu Loading

What's in this PR?

Why?

Checklist

weierophinney commented Apr 4, 2023

simPod left a comment • edited Loading

Choose a reason for hiding this comment

weierophinney commented Apr 5, 2023

simPod left a comment

Choose a reason for hiding this comment

weierophinney commented Apr 6, 2023 • edited Loading

simPod commented Apr 6, 2023

weierophinney commented Apr 6, 2023

simPod commented Apr 6, 2023

dbu commented Apr 7, 2023

weierophinney commented Apr 7, 2023

dbu commented Apr 10, 2023

Nyholm left a comment

Choose a reason for hiding this comment

dbu commented Apr 17, 2023

weierophinney commented Apr 17, 2023

weierophinney commented Apr 18, 2023

dbu commented Apr 19, 2023

weierophinney commented Apr 19, 2023

dbu commented Apr 20, 2023

weierophinney commented Apr 21, 2023

dbu commented Apr 28, 2023

weierophinney commented Apr 4, 2023 •

edited by dbu

Loading

simPod left a comment •

edited

Loading

weierophinney commented Apr 6, 2023 •

edited

Loading