-
Notifications
You must be signed in to change notification settings - Fork 150
Conversation
Oh. It looks like this PR is working as expected. There are just some cs fixes needed |
Picking this up for checking while I'm on travel tomorrow 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted a few minor changes for consistency.
.travis.yml
Outdated
@@ -9,7 +9,7 @@ cache: | |||
env: | |||
global: | |||
- COMPOSER_ARGS="--no-interaction" | |||
- COVERAGE_DEPS="php-coveralls/php-coveralls" | |||
- DEPENDENCIES="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, we don't need to pre-define globals; the shell will resolve it to ''
if not defined.
.travis.yml
Outdated
@@ -62,7 +68,7 @@ install: | |||
- if [[ $TRAVIS_PHP_VERSION =~ ^5.6 ]]; then travis_retry composer update $COMPOSER_ARGS --with-dependencies $LEGACY_DEPS ; fi | |||
- if [[ $DEPS == 'latest' ]]; then travis_retry composer update $COMPOSER_ARGS ; fi | |||
- if [[ $DEPS == 'lowest' ]]; then travis_retry composer update --prefer-lowest --prefer-stable $COMPOSER_ARGS ; fi | |||
- if [[ $TEST_COVERAGE == 'true' ]]; then travis_retry composer require --dev $COMPOSER_ARGS $COVERAGE_DEPS ; fi | |||
- if ! [ -v "$DEPENDENCIES" ]; then travis_retry composer require --dev $COMPOSER_ARGS $DEPENDENCIES ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become if [[ $DEPENDENCIES != '' ]]
to be consistent with our other checks.
Also, please rename it to INTEGRATION_DEPS
so we know what kind of dependencies we're defining and using here.
test/Integration/RequestTest.php
Outdated
<?php | ||
/** | ||
* @see https://github.com/zendframework/zend-diactoros for the canonical source repository | ||
* @copyright Copyright (c) 2015-2017 Zend Technologies USA Inc. (http://www.zend.com) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this line in all new files to:
* @copyright Copyright (c) 2018 Zend Technologies USA Inc. (https://www.zend.com)
(Copyright extends from when the file is introduced. Also, updates to use SSL-enabled URL.)
- Changes `$DEPENDENCIES` to `$INTEGRATION_DEPS` in `.travis.yml`, and changes conditional in `install` script to check for a non-empty value. - Updates license docblock in new files.
@Nyholm I've pushed the changes I requested, and am now working on fixes for the flagged errors. |
Awesome. Thanks! |
PSR-7 requires a string or array of strings; if an array is provided, it cannot be empty.
Doing so ensures that when we merge values via `withAddedHeader()`, we retain the original values when key collissions might normally occur. Fixes compatibility with PSR-7.
Per the PSR-7 spec, should only allow arrays, objects, or null values.
Previously, a lazy `if ($password)` check meant that a value of `0` would be ignored; it now checks more specifically for `if (null !== $password)`.
`MessageTraitTest` had two tests that were for behavior not supported by PSR-7: - `testHeaderExistsIfWithNoValues` - `testHeaderWithNoValues` As the integration tests demonstrate, these were not valid scenarios. As such, the tests are removed.
Forward port #325 Conflicts: CHANGELOG.md
You are great. Thank you! |
I noticed that this library does not fully comply with psr7.
I’ve added integration test to show where it is failing.
(I’m at the airport atm, will complete this PR later)