Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump PSR-7 dependency to allow usage with v2 #42

Merged
merged 3 commits into from
May 9, 2023

Conversation

weierophinney
Copy link
Member

@weierophinney weierophinney commented May 9, 2023

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

Description

This patch bumps the psr/http-message version to allow v2.0, and also bumps the testing requirements to allow either Diactoros 2 or 3 versions. Internally, since we are only consuming PSR-7, there are no code changes required. However, due to the fact that v2 has explicitly declared type hints, a number of locations in the tests where we were defining mocks needed updates to ensure that method parameters and return types used conformed to the specification.

Additionally, when running QA checks, I discovered (a) a test that was using deprecated PHPUnit functionality (fixed to be forwards compatible), and (b) 3 unused test asset classes (now removed).

Replaces #40

Bumps the psr/http-message dependency to allow v2.

Test changes are due to the fact that a number of mocks were not returning the correct value, and with RTH defined in v2, these were now raising errors.
With the changes, they are now more correct, and test assertions continued to pass without issue.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Each of the test cases updated were asserting the type of an object returned from one of the shipped functions.
However, each of these functions has return type declarations, making the assertions redundant.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
- Updated Psalm config to add configuration for upcoming version (findUnusedBaselineEntry, findUnusedCode)
- We had three test asset classes that were no longer used.
- A test was using deprecated PHPUnit functionality; rewrote in a way that will be forwards compatible.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Copy link
Member

@boesing boesing left a comment

Choose a reason for hiding this comment

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

LGTM, I'd say we could drop support for psr/http-message 1.0 and require at least 1.1 so that we can totally depend on diactoros v3, but thats not a hard requirement from me here.

},
"require-dev": {
"laminas/laminas-coding-standard": "~2.5.0",
"laminas/laminas-diactoros": "^2.22",
"laminas/laminas-diactoros": "^2.25 || ^3.0",
Copy link
Member

Choose a reason for hiding this comment

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

I'd say lets drop support for http-message v1.0 and bump it to 1.1, then we can just require diactoros v3 and don't have to hassle with v2 here.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

My main reason to keep 1.0 support was because I'm not 100% certain which of our components depend explicitly on Diactoros, and this would allow usage with v2 or v3 of that library until those are updated.

I think in 3.11 we could drop 1.0 support.

@weierophinney weierophinney merged commit d45eec2 into laminas:3.10.x May 9, 2023
@weierophinney weierophinney deleted the feature/psr-7-bump branch May 9, 2023 14:23
@weierophinney weierophinney mentioned this pull request May 9, 2023
1 task
Sign up for free to 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.

2 participants