clue / reactphp-soap

Simple, async SOAP webservice client, built on top of ReactPHP.
https://clue.engineering/2020/announcing-reactphp-soap-2
MIT License
64 stars 25 forks source link

Added the possibility to use local WSDL files #11

Closed floriansimon1 closed 8 years ago

floriansimon1 commented 9 years ago

The previous pull request contained bad commits. This one should be better.

clue commented 9 years ago

Awesome, I like the new feature!

However, I'm not sure about the method names createClient() vs createClientFromWsdl(). What's your take on this?

floriansimon1 commented 9 years ago

However, I'm not sure about the method names createClient() vs createClientFromWsdl(). What's your take on this?

I don't know. I can only agree with you on the fact that createClientFromWsdl is not the smartest name you can give to that method.

Possible candidates :

What do you prefer ? Do you have any other idea ?

clue commented 9 years ago

What do you prefer ? Do you have any other idea ?

IMO the original createClient() is already a bad choice, simply because it does not actually return a Client instance in the first place. It returns a Promise that resolves to Client.

Also, once we aim for #5, we should make it more obvious which method uses WSDLs.

This means we should aim to rename the original method name (in a separate PR!). Something like createDeferredClientWsdlDownload($uri) might make sense?

Your new method does in fact return a Client instance for the given WSDL contents. As such, my personal vote would be something like createClientWsdl($wsdl), but I'm very open to your suggestions.

Can you update your PR with the new method name? :+1: Also, I think we should emphasize the difference to the existing method (documentation).

floriansimon1 commented 9 years ago

I don't know if renaming the original function makes sense. This creates a compatibility break which you might want to avoid. The fact that createClient does not return a client is not a problem to me. It says that it's creating a client, not that it's returning it. I'm kind of biased though, I'm used to working with promises for everything :)

If you're ready to do it anyway, which I can understand, I would rename it to something like createFromWsdlUrl. It'd make it clearer that it's loading a remote WSDL file, which to me implies that it's returning a promise.

I'd rename the createClientFromWsdl method I'm writing to createClientFromDefinition make it clear that the method creates a client using a preloaded definition file (WSDL).

The fact that createClient does not return a client is not a problem to me. It says that it's creating a client, not that it's returning it. I'm kind of biased though, I'm used to working with promises for everything :)

I can't say I particularly like createClientWsdl, because, it might let people think that it generates a WSDL file, which would be confusing IMO.

It's all up to you !

clue commented 9 years ago

I don't know if renaming the original function makes sense.

You're raising some very valid points here :+1: You're also right in that it probably doesn't make much sense to discuss this in this PR :) As such, I've just filed #15, so let's address these there :+1:

Let's get back on topic here :)

In fact, I like the createClientFromWsdl() method name as it accurately describes what it does :+1:

Would you care to update the the documentation as to how this new method differs from the existing, dreaded createClient() method? Thanks!

floriansimon1 commented 9 years ago

It's already added in commit 7a5c5a8. It is OK for you ?

clue commented 8 years ago

Much appreciated, thanks @floriansimon1! :+1: