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

Fix for timeout config parameter validation #168

Merged
merged 3 commits into from Jan 22, 2019
Merged

Fix for timeout config parameter validation #168

merged 3 commits into from Jan 22, 2019

Conversation

klimser
Copy link
Contributor

@klimser klimser commented Jan 11, 2019

Validation conditions for integer or numeric timeout was mutually exclusive, fixed.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@klimser We need add unit tests to cover these changes.

@klimser
Copy link
Contributor Author

klimser commented Jan 11, 2019

@webimpress There are already existed tests that were failed and now passed:

  • test/Client/CurlTest/testPassValidTimeout
  • test/Client/SocketTest/testPassValidTimeout

@michalbundyra
Copy link
Member

@klimser

There are already existed tests that were failed ...

3 days ago builds on master and develop pass and nothing has been changed since then. How do you mean that these test were failing? Where?

@klimser
Copy link
Contributor Author

klimser commented Jan 11, 2019

@klimser

There are already existed tests that were failed ...

3 days ago builds on master and develop pass and nothing has been changed since then. How do you mean that these test were failing? Where?

I've updated my project to current release -> project was destroyed -> I found the reason, place in code ->I found that changes were made with tests -> I ran tests -> I got exception -> I fixed a bug -> Tests passed -> I created a pull request.

@klimser
Copy link
Contributor Author

klimser commented Jan 11, 2019

@webimpress

@klimser

There are already existed tests that were failed ...

3 days ago builds on master and develop pass and nothing has been changed since then. How do you mean that these test were failing? Where?

These tests are run only when TESTS_ZEND_HTTP_CLIENT_BASEURI set to true and skipped otherwise, maybe it's the reason?

@samsonasik
Copy link
Contributor

actually, the ! is_int($connectTimeout) is not needed as the ! is_numeric() already cover it, and later will always be casted to (int) $connectTimeout in next line.

@klimser
Copy link
Contributor Author

klimser commented Jan 14, 2019

actually, the ! is_int($connectTimeout) is not needed as the ! is_numeric() already cover it, and later will always be casted to (int) $connectTimeout in next line.

Agree. Fixed.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@klimser I had another look on it and I can confirm it was a bug, and your fix is correct. We haven't seen it in tests, because as you noticed these test were skipped. It would be nice to enable them on Travis.

Anyway - LGTM 👍 Thanks!

@weierophinney weierophinney merged commit a734185 into zendframework:master Jan 22, 2019
weierophinney added a commit that referenced this pull request Jan 22, 2019
weierophinney added a commit that referenced this pull request Jan 22, 2019
weierophinney added a commit that referenced this pull request Jan 22, 2019
Forward port #168

Conflicts:
	CHANGELOG.md
@weierophinney
Copy link
Member

Thanks, @klimser.

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

Successfully merging this pull request may close these issues.

None yet

4 participants