[2.x] Adds strict types using Phpstan #44
Conversation
src/Support/Fakery/Fakery.php
Outdated
| /** | ||
| * @param array<string, Closure(Request): Response|Response>|null $callback | ||
| */ | ||
| public function fake(array $callback = null): void |
There was a problem hiding this comment.
Does this need to be ?array?
There was a problem hiding this comment.
According to how it was/is implemented below, yes
| /** | ||
| * Make a request and return its response. | ||
| */ | ||
| public function call(string $method, mixed $body): mixed; |
There was a problem hiding this comment.
maybe better to change mixed $body to array $arguments?
That way it is compatible with how ext-soap works internally and with php-soap/engine:
- https://www.php.net/manual/en/soapclient.call.php
- https://github.com/php-soap/engine/blob/main/src/Engine.php
It allows for multi-params requests (see #45)
| $this->registerRay(); | ||
| $this->app->bind(Request::class, fn (Application $app) => new SoapPhpRequest( | ||
| $app->make(Builder::class), | ||
| new DecoratedClient() |
There was a problem hiding this comment.
So if I understand this correctly, changing a client would mean overwriting the bind to the request class?
The request class itself should be compatible over different clients?
Did you already have a vision on how you would like people to configure different clients / requests?
Would that be something on the soap facade or rather in service config?
| /** | ||
| * @param array<string, mixed>|Soapable $body | ||
| */ | ||
| public function call(string $method, array|Soapable $body = []): Response; |
There was a problem hiding this comment.
Same as for the Client contract : it is better to make it an array of parameters imo or a variadic
|
|
||
| private function client(): Client | ||
| { | ||
| return $this->client |
There was a problem hiding this comment.
This structure is a bit strange since you can change options in between requests, but the decorated client itself uses a memoized versionof the client. Only the headers will be changed since the options and endpoint might have been set.
Either way : you want to avoid constructing the internal SoapClient twice, since it will load the wsdl again as well. which canbe a slow task
| /** | ||
| * @internal | ||
| */ | ||
| final class DecoratedClient implements Client |
There was a problem hiding this comment.
This name is a bit strange to me : sure, it decorates a built-in SoapClient.
Maybe better to name it after what it decorates? That could make it easier if you want to provide more clients
| return $this->client()->__getFunctions(); | ||
| } | ||
|
|
||
| public function __get(string $name): mixed |
There was a problem hiding this comment.
what is this being used for exactly? There are no public properties on the client IIRC.
| } | ||
|
|
||
| public function to(string $endpoint) | ||
| public function to(string $endpoint): Request | ||
| { | ||
| return (clone $this->request) |
There was a problem hiding this comment.
since it clones the internal request, what happens if you call Soap::to twice with different wsdls?
Does it incorrectly use the memoized client decorated client from inside the Request class?
Reason why I ask : many coorporations split their soap services into multiple WSDL files for you to implement.
|
|
||
| use SoapHeader; | ||
|
|
||
| interface Client |
There was a problem hiding this comment.
Pretty sure it is possible to create a client using phpro/soap-client that implements this interface.
| * | ||
| * @return array<int, string> | ||
| */ | ||
| public function getFunctions(): array; |
There was a problem hiding this comment.
Our php-soap engine returns more information about the functions than a regular string.
https://github.com/php-soap/engine/blob/main/src/Metadata/Metadata.php
We could parse them back to a regular int, string - but it means you will loose some information.
Alternatively we could add a generic for determing what kind of information this function will return?
|
|
||
| interface Client | ||
| { | ||
| public function setEndpoint(string $endpoint): static; |
There was a problem hiding this comment.
Not sure if the setters in this interface are a good idea though. See further down this code review - it allows for some strange structures.
| public function withDigestAuth(string $login, string $password): self; | ||
|
|
||
| public function withHeaders(Header ...$headers): self; | ||
|
|
There was a problem hiding this comment.
This request is a bit shaped around the existing soap client.
Did you already think about how more advanced options could be added to a request? For example http middleware etc, ... - which are not possible with the regular soap client?
For 2.x, I'd like to introduce strict type hinting.
The reasoning is fairly straightforward: better IDE support, a smaller chance of bugs and better integration for other users using PhpStan.
This also gives us a great opportunity to think about next steps as regards Http drivers (#36), unified interfaces and more.