Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Fix invalid default method #162

Closed
wants to merge 1 commit into from

Conversation

snapshotpl
Copy link
Contributor

Currently, when method is not provided it's set as empty string. It's invalid state, because it's not possible to set new method (using withMethod) as empty string. It's also other possible invalid state - we can set new method as null, which is not compatible with RequestInterface.

@weierophinney weierophinney added this to the 1.4.0 milestone Sep 7, 2016
@weierophinney weierophinney self-assigned this Sep 7, 2016
@weierophinney
Copy link
Member

This is a slight BC break in that toString() can now raise an exception. As such, I'm going to push it to the next minor release.

@weierophinney weierophinney modified the milestones: 2.0.0, 1.4.0 Jan 5, 2017
@weierophinney
Copy link
Member

Hi, @snapshotpl!

I've been reviewing this in a bit more detail, and I've decided to push it to a future 2.0 release.

The reason I'm pushing it to a new major release is because it breaks existing assumptions codified in the tests, particularly around the fact that we previously allowed a client-side request to have a nullable request method; changing this assumption, while a net-positive, causes potential breakage for existing users who may have depended on this "feature".

(Interestingly, PSR-7 does not indicate that there MUST be a value present for the HTTP method, only that getMethod() must return a string. The current implementation does that, except in one case — we currently allow null, which we should not. More generally, PSR-7 does not dictate that a given instance MUST have a valid state, and there are a variety of ways to create an instance with invalid state, including omitting the URI!)

@weierophinney weierophinney changed the base branch from master to release-2.0 May 29, 2018 16:58
weierophinney added a commit that referenced this pull request May 29, 2018
@weierophinney
Copy link
Member

Merged with #302.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants