Skip to content

Some changes to achieve compatibility with php8.0 #4

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.

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

Closed
wants to merge 5 commits into from

Conversation

rkrx
Copy link

@rkrx rkrx commented May 27, 2021

  • Fixed some issues reported by phpstan (missing return types)
  • Composer: Raised phpunit to ^8.0
  • Composer: php-http/mock-client is now regularly provided in the latest version. Min = 1.4.1, which is compatible from 7.1 up to 8.0
  • Composer: phpstan is now regularly provided in the latest version
  • Composer: Added script section for phpstan
  • Added meaningful phpstan.neon

rkrx added 4 commits May 27, 2021 11:28
- Composer: Raised phpunit to ^8.0
- Composer: php-http/mock-client is now regularly provided in the latest version. Min = 1.4.1, which is compatible from 7.1 up to 8.0
- Composer: phpstan is now regularly provided in the latest version
- Composer: Added script section for phpstan
- Added meaningful phpstan.neon
…of method Psr\Http\Message\StreamFactoryInterface::createStream() expects string, string|false given."
…returnOrder with type JsonSerializable is not subtype of native type Dhl\Sdk\Paket\Retoure\Model\RequestType\ReturnOrder."
@mam08ixo
Copy link
Contributor

mam08ixo commented May 27, 2021

Thank you for your contribution @rkrx

Your fork currently fails to install, caused by the change to the php-http/mock-client version requirement:

Problem 1
  - The requested package psr/http-client-implementation could not be found in any version, there may be a typo in the package name.

Another installation issue occurs when using PHP 7.1, which is supported by this package.

Problem 2
  - phpunit/phpunit 8.5.9 requires php ^7.2 -> your PHP version (7.1.33) does not satisfy that requirement.

Our packages are built to support the same PHP versions for dev and project use. That is why the dev requirements are locked to older versions. We cannot validate PHP version support if the quality tools (dev requirements) fail to install/run in that environment.

The solution is to either define the dev requirements so that all the range from 7.1 to 8.0 is supported or create a new package version that drops PHP 7.1 support.

However, the mock client issue is still a blocker for quite a while now:

@rkrx
Copy link
Author

rkrx commented May 27, 2021

Your fork currently fails to install, caused by the change to the php-http/mock-client version requirement:

Ah ok, I was wondering. This was also a problem with the previous version IIRC.
There is actually a discussion about that as well. After that, I also tend to think that php-http/mock-client should not be an HTTP client implementation. But I don't want to make a final commitment on this yet.

I added guzzle while I was testing php 8.0 version support locally.

Another installation issue occurs when using PHP 7.1, which is supported by this package.

Yes, that is annoying. PHPUnit only supports a certain range of versions - all officially supported php versions.

This is not a direct problem when using dhl/sdk-api-bcs-returns as a dependency as the require-dev-part wont be evaluated in that case. Only if you do a first level install.
If you use some CI-tool, you have to install phpunit 7 for php7.1 via script and override the composer.json dependency. There is no other known way to solve this scenario.

@mam08ixo
Copy link
Contributor

I added guzzle while I was testing php 8.0 version support locally.

That is impossible. You cannot execute the tests with no mock client. Replacing the mock client by a real HTTP client is not an option.

As mentioned before, I am open to dropping PHP 7.1 support in favor of PHP 8.0 – pretty sure one day that will happen anyway.

My point is, we have to put this on hold until a PHP 8 and PSR-18 compatible mock client becomes available.

@rkrx
Copy link
Author

rkrx commented May 27, 2021

That is impossible. You cannot execute the tests with no mock client. Replacing the mock client by a real HTTP client is not an option.

Wait, you missunderstood me. You've mentioned this:

The requested package psr/http-client-implementation could not be found in any version, there may be a typo in the package name.

If you add any http-client (guzzle or some other) this error-message goes away. This is what I did. I removed this dependency once I was ok with my changes.

As mentioned before, I am open to dropping PHP 7.1 support in favor of PHP 8.0 – pretty sure one day that will happen anyway.

You dont have to. All you have to do is to have some script to install phpunit 7 instead of 8 while in CI-test-mode for php 7.1.

All projects out there using dhl/sdk-api-bcs-returns wont notice, since require-dev will be ignored for third-party depedencies and this is a problem only at require-dev-level.

Not sure if phpunit/phpunit: >= 7.0 && < 9.0 would do it in CI-env

@mam08ixo
Copy link
Contributor

I did understand at second read but that's a hack. I will not add a package to the library without actually using it, just to match the virtual package requirements.

I will also not make modifications to the CI (which builds dozens of projects in various runtime variations) to, under some circumstances, use tools that are not listed in the dev requirements.

We will evaluate alternative mock clients if there is no progress with php-http/mock-client.

Again, thank you for your contribution. I hope we can review & merge this PR at a later date.

@rkrx
Copy link
Author

rkrx commented May 27, 2021

I just made a temporary meta-package available for php-http/mock-client, which basically just adds a provide-section for psr/http-client-implementation.

"require": {
	"php-http/mock-client": ">= 1.4.1"
},
"provide": {
	"psr/http-client-implementation": ">= 1.0"
}

And I made two changes for the test-cases.

@powli
Copy link
Member

powli commented May 18, 2022

Closing this as this package is compatible with PHP 8.0 as of v2.1.0

@powli powli closed this May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants