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.

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

Update Symfony HttpClient implementation for v6.* and PHP to 8.0 #96

Merged
merged 7 commits into from
Sep 4, 2022
Merged

Update Symfony HttpClient implementation for v6.* and PHP to 8.0 #96

merged 7 commits into from
Sep 4, 2022

Conversation

NicolasDievart
Copy link
Contributor

Hi !

As of Symfony/HttpClient 5.3, the withOptions function was introduced and it is, therefore, mandatory, if you want to upgrade to the last version of Symfony/HttpClient

Symfony 6 also requires PHP 8 (and for the implementation of the withOptions function with the static return), I bumped the minimum required version to 8.0. I can go to 8.1 if you prefer, but don't know the other usages of Ganesha (other than HttpClient) and don't want to break anything.

@ackintosh
Copy link
Owner

Awesome, thank you for this PR @NicolasDievart!

This PR drops PHP 7.x support, so I'm planning to release this PR as a major version up, v3.0.0. ✨


Note for self:

PHP 7.4 is in security support (until 28 Nov 2022): PHP: Supported Versions

I think it's okay to drop 7.x as the 7.x reaches EOL in 3 months.

@ackintosh
Copy link
Owner

@NicolasDievart

Oh, the tests on GitHub Actions are failing...

PHP 7.x will no longer supported since this pull request, could you please remove 7.x from this GitHub Actions workflow?

@NicolasDievart
Copy link
Contributor Author

Hi 👋

Sorry, I couldn't test the github action on my side, just runned the test against PHP 8 on my local env.

I fear I can't just drop 7.4 from the github action after a small investigation.
I saw you comment https://github.com/ackintosh/ganesha/blob/master/.github/workflows/tests.yml#L69 :

      # https://github.com/php-coveralls/php-coveralls#github-actions
      - name: Upload coverage results to Coveralls
        # NOTE: For now we would run this step only on php7.4 due to the error on php8.
        # https://github.com/ackintosh/ganesha/runs/4375377952?check_suite_focus=true
        # > PHP Fatal error:  Declaration of PhpCoveralls\Bundle\CoverallsBundle\Console\Application::getDefinition() must be compatible with ...
        if: ${{ matrix.php-version == '7.4' }}

https://github.com/php-coveralls/php-coveralls seems to be compatible now

However, the php-vcr/phpunit-testlistener-vcr is still not compatible with PHP 8.
There is an old open PR here : php-vcr/phpunit-testlistener-vcr#38
But seems to be no plan to merge/tag this
We could use a fork if you want to keep this package https://github.com/CoverGenius/phpunit-testlistener-vcr ?

@ackintosh
Copy link
Owner

However, the php-vcr/phpunit-testlistener-vcr is still not compatible with PHP 8.
There is an old open PR here : php-vcr/phpunit-testlistener-vcr#38
But seems to be no plan to merge/tag this

Yeah, regarding phpunit-testlistener-vcr, we already using a fork. It works well for PHP7 and PHP8. 👍

@coveralls
Copy link

coveralls commented Aug 27, 2022

Coverage Status

Coverage increased (+0.06%) to 92.514% when pulling ee4f756 on NicolasDievart:update-symfony-http-client-with-options-and-php into 5792a96 on ackintosh:master.

@ackintosh
Copy link
Owner

Coverage Status

Coverage decreased (-0.5%) to 91.939% when pulling b38a161 on NicolasDievart:update-symfony-http-client-with-options-and-php into 5792a96 on ackintosh:master.

The coverage decrease seems due to here 👀 : https://coveralls.io/builds/52000155/source?filename=src%2FGanesha%2FGaneshaHttpClient.php#L101

@NicolasDievart
It would be great to add a unit test for GaneshaHttpClient::withOptions(). 🙏 FYI: The test file for GaneshaHttpClient is here.

@NicolasDievart
Copy link
Contributor Author

I added unit test for the withOptions method (inspired by the test from HttpClient directly https://github.com/symfony/symfony/blob/6.2/src/Symfony/Contracts/HttpClient/Test/HttpClientTestCase.php#L1122)

I added an http call on the server docker container, don't know if useful or not ?

@ackintosh
Copy link
Owner

ackintosh commented Aug 30, 2022

Nice. ✨ I have made a small change to do a more strict test: aab5494

@ackintosh
Copy link
Owner

ackintosh commented Aug 30, 2022

The test with --prefer-lowest option was failed.

https://github.com/ackintosh/ganesha/runs/8085719168?check_suite_focus=true

  1. Ackintosh\Ganesha\GaneshaHttpClientTest::buildWithOptions
    Error: Call to undefined method Symfony\Component\HttpClient\CurlHttpClient::withOptions()

As far as I investigated, withOptions() is implemented since v5.3. So we should align our requirement:

- "symfony/http-client": "^4.3|^5.0|^6.0",
+ "symfony/http-client": "^5.3|^6.0",

because `withOptions()` is implemented since v5.3
@NicolasDievart
Copy link
Contributor Author

Thank you very much for your help 👍

@ackintosh ackintosh merged commit fa6e940 into ackintosh:master Sep 4, 2022
@ackintosh
Copy link
Owner

Released this as v3.0.0. 🎉

Again, thank you for your contribution @NicolasDievart!

@ackintosh
Copy link
Owner

By the way, please consider adding your company to the list if you don't mind. 🙂

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.

None yet

3 participants