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

Updated to work with PHP8's SoapClient #4

Closed
wants to merge 3 commits into from
Closed

Updated to work with PHP8's SoapClient #4

wants to merge 3 commits into from

Conversation

boboldehampsink
Copy link
Contributor

@boboldehampsink boboldehampsink commented Mar 4, 2021

@boboldehampsink boboldehampsink changed the title Fix PHP8 compatibility Updated to work with PHP8's SoapClient Mar 4, 2021
@MegaChriz
Copy link
Owner

Thanks for your contribution! It does raise the minimum PHP to version 8. I think it's a little too early to require that PHP version. I'm not using PHP 8 in any of my projects yet.

I think to support both PHP 7 and PHP 8, we need to conditional define classes and interfaces:

if (version_compare(PHP_VERSION, '8.0.0') >= 0) {

  /**
   * Provides an interface for handling Soap Requests.
   */
  interface SoapClientInterface {

    /**
     * Performs A SOAP request.
     *
     * {@inheritdoc}
     */
    public function __doRequest(string $request, string $location, string $action, int $version, bool $oneWay = false);

  (...)
}
else {

  /**
   * Provides an interface for handling Soap Requests.
   */
  interface SoapClientInterface {

    /**
     * Performs A SOAP request.
     *
     * {@inheritdoc}
     */
    public function __doRequest($request, $location, $action, $version, $one_way = 0);

  (...)
}

It's not pretty, but I think it's the easiest solution. I'm open to other ideas though.

@boboldehampsink
Copy link
Contributor Author

Yeah I fully agree, there is no other way to make it work for both versions sadly.

@boboldehampsink
Copy link
Contributor Author

@MegaChriz fixed to work that way

@boboldehampsink
Copy link
Contributor Author

@MegaChriz any updates?

@MegaChriz
Copy link
Owner

Oops, forgot about this one. In composer.json PHP 8 still gets required. The requirement should stay PHP 7 for now.

@boboldehampsink
Copy link
Contributor Author

@MegaChriz fixed

@MegaChriz
Copy link
Owner

Thanks, now I would like to add running tests with PHP 8, but I see lots have changed with Travis. It's now travis-ci.com instead of travis-ci.org. I don't use Travis actively for any other projects, so that would take me some time to figure out, I guess.

@boboldehampsink
Copy link
Contributor Author

Hi @MegaChriz, any news?

@MegaChriz
Copy link
Owner

I've tried to update the travis config now in branch "202111_travis". It's failing tests on PHP 7.4. To get these tests passing on that version, I need to make the tests compatible with PHPUnit 8.2.3+ and drop support for PHP versions lower than PHP 7.2 (because PHPUnit requires at least that version).
See sebastianbergmann/phpunit#3728

@MegaChriz
Copy link
Owner

Your changes are merged in!

Things I did:

  • Updated the tests for PHPUnit 8.5. Tests passed on PHP 7.2 till PHP 8.0.
  • PHP 8.0 tests should have failed, so I added minimal test coverage for the SoapClient class. Then tests failed on PHP 8.
  • Added your changes, with minimal changes to code formatting.

Tests passed on PHP 8! And they still also passed on the other PHP versions that were tested. So the issue is resolved now. 😀
Sorry that it took that long. I'm not so used on working on github, I'm more focussed on doing open source work on drupal.org.

@MegaChriz MegaChriz closed this Nov 30, 2021
@boboldehampsink
Copy link
Contributor Author

Appreciate this a lot @MegaChriz 👍

@boboldehampsink
Copy link
Contributor Author

Don't forget to tag a new release :-)

@boboldehampsink
Copy link
Contributor Author

@MegaChriz could you also tag a release on packagist? https://packagist.org/packages/megachriz/afasprofit

@MegaChriz
Copy link
Owner

I've created a new release and did an update on packagist. Thanks for reminding me. I hadn't thought of updating packagist. Apparently, there is a way to auto-update packagist using a GitHub Hook.

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

2 participants