Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

AbstractSoapClientBase should not define static SoapClient instance #15

Closed
cheindl opened this issue Oct 14, 2017 · 7 comments
Closed

Comments

@cheindl
Copy link

cheindl commented Oct 14, 2017

When having multiple subclasses of AbstractSoapClientBase with different WSDL endpoints the static instance of SoapClient is shared between the subclasses. As a SoapClient instance is bound to one WSDL endpoint you are not able to call different endpoints without completely reinitializing AbstractSoapClientBase-subclass.

AbstractSoapClientBase::soapClient should not be defined on that level. Subclasses of AbstractSoapClientBase should be responsible to define a static SoapClient.

An alternative would be to use an instance member for SoapClient instead of a static member. But this implies that SoapClientInterface needs to be changed aswell.

@mikaelcom
Copy link
Member

Current situation
To be honest, the behaviour of SoapClientInterface and AbstractSoapClientBase is surely not perfect.

Not a long time ago, in the WsdlToPhp/PackageGenerator#97 issue, it has been brought to my knowledge that a better SoapClient implementation had been created at https://github.com/phpro/soap-client and I can't deny that it provides strong features and a strong implementation around the SoapClient class.

This is why I'm struggling between:

  • implementing my own SoapClient as it is provided currently basically (and at least improving it)
  • improving the usage of any another SoapClient implementation by facilitating it

My opinion is that it's useless to create yet another SoapClient implementation especially if there is another one that is better and is liked by others.

Future changes
Knowing this, I think the best decision would be to:

  • remove the staticness of the SoapClientInterface::getSoapClient method
  • maybe remove the SoapClientInterface::setSoapHeader as it might be useful for everyone to declare this (this has been a personal choice to add this method, it would be kept in the AbstractSoapClientBase class, this class would always be provided as the default SoapClient wrapper)
  • change SoapClientInterface::saveLastError($methodName, \SoapFault $soapFault) to SoapClientInterface::saveLastError(\SoapFault $soapFault)
  • change SoapClientInterface::getLastError() : array to SoapClientInterface::getLastError() : \SoapFault
  • remove the staticness of the AbstractSoapClientBase::getSoapClient method
  • remove the staticness of the AbstractSoapClientBase::$soapClient property

This would effectively impacts the PackageGenerator project as it would need to change the static calls to non static calls to the SoapClient instance plus minor changes.

All these changes would require to create a new major release for both projects as it would break current implementations.

Gains of the future changes

  • As you can already define the used SoapClient implementation with the set(Option)SoapClientClass (--soapclient), you'll be able to more easily create your own base SoapClient class from the ground (ex : SoapClientbase) that would define fewer methods from the SoapClientInterface.
  • Avoid conflict between calls
  • Get more easily the SoapFault if there is any
  • Use the https://github.com/phpro/soap-client implementation certainly more easily

What do you think? Am I missing more improvements about this issue?

@neyaoz
Copy link
Contributor

neyaoz commented Mar 11, 2018

@mikaelcom Is there any update? Because it is not possible to create instances for two different services on the same flow.

@mikaelcom
Copy link
Member

Hi, I'm beginning my new life as of tomorrow so updates are coming soon, I'll have more time to work on my open source projects, I can't tell right now exactly when but soon,

Currently, you can override the SoapClientBase as the SoapClient is used with static:: so you can implement you own logic, basic sample at https://github.com/WsdlToPhp/PackageEws365, in the meantime...

@neyaoz
Copy link
Contributor

neyaoz commented Mar 14, 2018

@mikaelcom Hi,

I had some time to solve this problem at least only in PackageBase. It works very well. #17

@mikaelcom
Copy link
Member

I doubt it works in a generated ServiceType class like this one,

This must be done at the same moment with the PackageGenerator release update and certainly in a major release for BC

@amari-at4
Copy link

amari-at4 commented Apr 17, 2018

The ideal solution is to use "Late Static BInding" but it's necessary to modify both projects "PackageBase" and "PackageGenerator"

On PackageBase, apply this patch to class AbstractSoapClientBase

On PackageGenerator it's necessary to change the generation of Get.php, to add a new property
protected static $soapClient;

For our project I have modified the Get.php files manually and now I can use two different web services at the same time without collisions.

This works on PHP 5.3.0 or later

@mikaelcom
Copy link
Member

Sorry for the delay/absence, I would prefer to change the property to a non-static property instead of adding the declaration of the protected static $soapClient property to every generated Service client as it puts code from the generic AbstractSoapClientbase that is not "necessary".

I'm going to have to create the new major version that introduces the $soapClient as non-static property. It will indeed impact both PackageBase and PackageGenerator. Hopefully, it will give me the courage to get back on both projects in order to improve them as I intended.

@amari-at4 feel free to give me more feedback if you want, thx for your participation

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants