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.

Sign up for GitHub

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

Jump to bottom

Better test if a Stream is closed #17

Merged
merged 1 commit into from
Sep 25, 2017
Merged

Better test if a Stream is closed #17

merged 1 commit into from
Sep 25, 2017

Conversation

danopz
Copy link
Contributor

@danopz danopz commented Sep 21, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets found in review of slimphp/Slim-Http#32
License MIT

What's in this PR? Why?

  • the @ suppresses warnings/errors which shoud be omitted
  • if a resource is closed it is not a resource anymore, so we can check it using is_resource

@danopz danopz mentioned this pull request Sep 21, 2017
Merged
@dbu
Copy link
Contributor

dbu commented Sep 25, 2017

sounds reasonable. not 100% sure if there is some reason why it was done like that before.

the test failures also exist on master. did you check if the specific test you modify in this PR succeeds with all frameworks?

Nyholm
Nyholm approved these changes Sep 25, 2017
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I think it looks good

@Nyholm
Copy link
Member

Nyholm commented Sep 25, 2017

Test fails because we are testing all psr7 implementations. It is only Nyholm/psr7 that pass.

@Nyholm Nyholm merged commit fd35a01 into php-http:master Sep 25, 2017
@Nyholm
Copy link
Member

Nyholm commented Sep 25, 2017

Thanks!

@danopz danopz deleted the stream-closed branch October 11, 2017 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danopz @dbu @Nyholm