Allow HTTPlug 2.0 #114

Jean85 · 2018-10-31T11:02:45Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	not sure yet
Deprecations?	no
Related tickets	-
Documentation	not sure it's needed yet
License	MIT

What's in this PR?

This PR (like php-http/discovery#115) is to test and allow HTTPlug 2 (and PSR-18)

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created (if not simply a bugfix)

To Do

Check and fix build failures

composer.json

GrahamCampbell · 2018-10-31T11:07:02Z

We should probably setup travis to test with 1.x and 2.x in each env.

dbu · 2018-10-31T11:09:38Z

have a composer --prefer-lowest build with php 7.3? or do we need more than that?

Jean85 · 2018-10-31T11:13:34Z

Build is failing because HttpMethodsClient::sendRequest is missing the return type, so it's not PSR-18 and HTTPlug 2 compatible.

How should I tackle this? I see two roads:

aim to a new major release compatible just with HTTPlug 2, and leave the resolution of conflicting versions to Composer
try to work some magic and use some backward-compat trick to avoid any breaking changes

I would prefer the former, since I don't see how we could implement the latter; it would work only on PHP 7.2+ which has parameter widening, but we will be out of luck in PHP 7/7.1

dbu · 2018-10-31T11:16:46Z

👍 for a new major version that only works with httplug 2. will also keep the testing simpler ;-)

sagikazarmark · 2018-10-31T11:35:34Z

New major version is OK, but that should go to a 2.x branch as we will probably have multiple, subsequent PRs.

Jean85 · 2018-10-31T11:39:44Z

@sagikazarmark perfectly ok with that! If you can create the branch I'll re-target this PR and add (instead of replace) the branch alias in composer.json

sagikazarmark · 2018-10-31T11:44:43Z

2.x branch is ready ;)

Jean85 · 2018-10-31T13:40:33Z

Current build failure are pretty strange, because they manifest only when restricting the deps with external deps: https://travis-ci.org/php-http/client-common/builds/448853233

Same config without additional requires are green. I'll try to compare the installed deps to find the differences.

Jean85 · 2018-10-31T14:07:02Z

I have different results between CI and local. The base problem seems to be in BatchExceptionSpec, because it's trying to test (with mocking or similar stuff) a class (BatchException) which is both final and an exception, so not doable.

What should I do? I'm not well versed with PHPSpec...

…antiator#20

Jean85

No longer WIP, please review!

I've added a few comments to clarify what I had to do to make the build green.

Jean85 · 2018-10-31T17:32:05Z

.travis.yml

@@ -19,17 +16,15 @@ branches:
 matrix:
    fast_finish: true
    include:
-        - php: 7.1
-          env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" DEPENDENCIES="doctrine/instantiator:^1.1"
+        - php: 7.0


This was previously on PHP 7.1 due to doctrine/istantiator 1.1, which is no longer required. I've added a conflict rule below.

Jean85 · 2018-10-31T17:32:29Z

.travis.yml

        - php: 7.0
        - php: 7.1
        - php: 7.2
-          env: COVERAGE=true TEST_COMMAND="composer test-ci"  DEPENDENCIES="henrikbjorn/phpspec-code-coverage:^1.0"
+        - php: 7.2
+          env: COVERAGE=true TEST_COMMAND="composer test-ci"  DEPENDENCIES="leanphp/phpspec-code-coverage"


This is a maintained fork of the previous package, which is now abandoned.

Jean85 · 2018-10-31T17:32:53Z

.travis.yml

@@ -44,9 +39,10 @@ matrix:
          env: STABILITY="dev"

    allow_failures:
-          # Latest dev is allowed to fail.
+        - php: 7.3


This is not working for now, but the issue is on Travis' side. Will work as soon as they fix it.

Jean85 · 2018-10-31T17:33:41Z

composer.json

        "php-http/message-factory": "^1.0",
        "php-http/message": "^1.6",
        "symfony/options-resolver": "^2.6 || ^3.0 || ^4.0"
    },
    "require-dev": {
-        "phpspec/phpspec": "^2.5 || ^3.4 || ^4.2",
+        "phpspec/phpspec": "^3.4 || ^4.2",


Dropping 2.x which was needed only to run under PHP 5.x

Jean85 · 2018-10-31T17:34:52Z

composer.json

        "guzzlehttp/psr7": "^1.4"
    },
+    "conflict": {


Those are lower bounds to avoid issues in CI builds. Should not affect --prefer-lowest.

--prefer-lowest will respect the conflict section. as will other packages, as discussed below. do we use the doctrine/instantiator or sebastian/comparator in our actual code, or only for tests? all things thests should be solved in require-dev rather than here, or we prevent installing this on some systems that are fine with those versions.

Both packages are needed by the test suite through PHPSpec/Prophecy; the dev deps tree should not have packages in common with the non-dev deps, so it should be fine. I preferred to use conflicts instead of requiring them explicitly because they are both indirect dependencies that could go away at any time.

the problem is that conflicts are also respected by applications using this package. meaning that if my project uses an older version of one of the libraries we mark as conflicting here will not be able to install this package because of the conflict.

conflict should only be used to mark versions of packages that may not be run together with this package. they should not be used to control what is installed for development. if this is all only about --prefer-lowest, i think we either need them in require-dev, or if you want to be more explicit, put them in the DEPENDENCIES of .travis.yml only for the lowest version build - that would be the most explicit way of doing it, imho.

joelwurtz · 2018-10-31T17:37:46Z

composer.json

        "guzzlehttp/psr7": "^1.4"
    },
    "conflict": {
        "doctrine/instantiator": "<1.0.5",
+        "phpspec/prophecy": "<1.8",


Does conflict interact with the global require ? Because it's only in conflict for the dev environment, does conflict-dev exist ?

Sadly conflict-dev does not exist, so we should keep the old behavior (minimum version in require dev), as the runtime library does not conflict with prophecy < 1.8. It's only the tests.

It's possible to simulate conflict-dev by making a dummy package that has exactly one conflict, then require said package in require-dev.

good catch. i'd prefer requiring prohecy ^1.8 in require-dev over creating a dummy package.

Ouch I didn't think of this issue! Maybe moving to require-dev is good enough?

Yes please set thoses packages into the require dev section instead of using conflict

dbu · 2018-11-01T07:57:51Z

composer.json

@@ -11,16 +11,21 @@
        }
    ],
    "require": {
-        "php": "^5.4 || ^7.0",
-        "php-http/httplug": "^1.1",
+        "php": "^7.0",


why not go directly to 7.1? afaik 7.0 is EOL already.

Because it's the same of PSR-18 and HTTPlug 2.

Im fine with 7.1. For psr18 it did not really matter. It was not using any php7.1 features.

as long as we agree that dropping 7.0 support is a minor change and we "just do it" if 7.0 becomes a pain for some reason, i am fine with allowing it for now ;-)

Nyholm · 2018-11-02T14:09:16Z

This PR require us to release a major version. Is this really what we want?

We could modify it to allow httplug 1 and 2. That would be an easier migration, right?

sagikazarmark · 2018-11-02T14:39:04Z

We could modify it to allow httplug 1 and 2. That would be an easier migration, right?

Technically that could work. We should probably consider it.

joelwurtz · 2018-11-02T15:28:51Z

Does not work since HttpMethodsClient is not final (so it's a BC break if we change the signature, exactly like the Guzzle package)

Nyholm · 2018-11-02T17:39:16Z

You are correct. I did not consider that this library actually have http-client implementations.

…

On 2 Nov 2018, at 17:28, Joel Wurtz ***@***.***> wrote: Does not work since HttpMethodsClient is not final (so it's a BC break if we change the signature, exactly like the Guzzle package) — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

Jean85 · 2018-11-04T22:30:18Z

Yep, exactly, it's a BC. But users of this lib will be able to require both versions if they want to support both HTTPlug versions.

BTW, what do you want me to do regarding the lower bound for Prophecy and doctrine/instantiator?

Jean85 · 2018-11-06T09:35:03Z

I've moved the conflicts to require-dev as per #114 (comment)

dbu

looks good to me, thanks!

i like that we use >= in the require-dev for the constraints, like this its clear its not regular dependencies.

Allow HTTPlug 2.0

c66e081

Nyholm reviewed Oct 31, 2018

View reviewed changes

composer.json Outdated Show resolved Hide resolved

Use the official syntax [skip CI]

35720b0

Jean85 mentioned this pull request Oct 31, 2018

Support HTTPlug 2 (and PSR-18) php-http/mock-client#26

Closed

Bump to a future major version; allow only HTTPlug 2

d7116bf

Jean85 added 2 commits October 31, 2018 12:41

Fix constraint collision in build

5dd4d9f

Fix all remaining client signatures

d99e7aa

Jean85 changed the base branch from master to 2.x October 31, 2018 11:45

Fix branch alias after PR retarget [skip CI]

af093a3

Jean85 force-pushed the patch-1 branch from b77d484 to af093a3 Compare October 31, 2018 11:47

Re-allow PHPSpec 3

36d79a7

mxr576 mentioned this pull request Oct 31, 2018

Add support for HTTPlug 2.0 apigee/apigee-client-php#24

Closed

Jean85 added 2 commits October 31, 2018 14:30

Add more jobs to the CI matrix

a8e1cb5

Fix build for 7.3

fae2083

Try to lock lower bound of deps to sebastian/comparator 2.0

8fc310c

Jean85 added 2 commits October 31, 2018 17:57

Require at least doctrine/instantiator 1.0.5 because of doctrine/inst…

a131600

…antiator#20

Require at least phpspec/prophecy 1.8 due to phpspec/prophecy#412

22c3573

Jean85 force-pushed the patch-1 branch from 2dac8f0 to 22c3573 Compare October 31, 2018 17:12

Jean85 added 3 commits October 31, 2018 18:14

Run --prefer-lowest on lowest PHP version (7.0)

9272148

Use leanphp/phpspec-code-coverage as a maintained fork

e54ceb7

Fix coverage extension load

8f65a3c

Jean85 force-pushed the patch-1 branch from 4776e86 to 8f65a3c Compare October 31, 2018 17:25

Jean85 changed the title ~~[WIP] Allow HTTPlug 2.0~~ Allow HTTPlug 2.0 Oct 31, 2018

Convert require into conflict where not needed

6e5c694

Jean85 commented Oct 31, 2018

View reviewed changes

joelwurtz reviewed Oct 31, 2018

View reviewed changes

dbu reviewed Nov 1, 2018

View reviewed changes

Jean85 mentioned this pull request Nov 5, 2018

Update to httplug 2.0? hwi/HWIOAuthBundle#1444

Closed

Move restrictions from conflict to require-dev

6ff43de

dbu approved these changes Nov 6, 2018

View reviewed changes

dbu merged commit b7e032b into php-http:2.x Nov 6, 2018

Jean85 deleted the patch-1 branch November 6, 2018 11:40

Jean85 mentioned this pull request Nov 6, 2018

Release 2.0 #115

Closed

5 tasks

Allow HTTPlug 2.0 #114

Allow HTTPlug 2.0 #114

Conversation

Jean85 commented Oct 31, 2018

What's in this PR?

Checklist

To Do

GrahamCampbell commented Oct 31, 2018

dbu commented Oct 31, 2018

Jean85 commented Oct 31, 2018

dbu commented Oct 31, 2018

sagikazarmark commented Oct 31, 2018

Jean85 commented Oct 31, 2018

sagikazarmark commented Oct 31, 2018

Jean85 commented Oct 31, 2018

Jean85 commented Oct 31, 2018

Jean85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Nov 2, 2018

sagikazarmark commented Nov 2, 2018

joelwurtz commented Nov 2, 2018

Nyholm commented Nov 2, 2018 via email

Jean85 commented Nov 4, 2018

Jean85 commented Nov 6, 2018

dbu left a comment

Choose a reason for hiding this comment