feat: Add support for to use a object with a union type #580
Conversation
src/Options/ClientOptions.php
Outdated
| use OptionsTrait; | ||
|
|
||
| private ?string $apiEndpoint; | ||
| public ?string $apiEndpoint; |
There was a problem hiding this comment.
This is a highly opinionated change. My believe is that the options objects should be mostly structural and have easy of use for the user and the developers.
If you are in need or wanting to use this object, using an IDE and start typing $options-> should give a quick auto completion that helps the user understand the different options. We should document these values if we decided to continue with this philosophy.
Due this being highly opinionated though, I am willing to return this to private.
There was a problem hiding this comment.
The only concern I have here is that if there's some logic being done in the setters (which is the case for a few of these), it would be lost in setting these manually, which may lead to some confusing behavior.
There was a problem hiding this comment.
I think if we DID do this, we should rewrite the class and just make it a POPO.
There was a problem hiding this comment.
After sitting on this for almost a year, I don't think we should make the properties public. My reasoning here is that
- you can access them already with array access (e.g.
$clientOptions['apiEndpoint']) - they do work in the setters (e.g.
setCredentialsFileloads the file contents)
efd7432 to
f07cbf5
Compare
bshaffer
left a comment
There was a problem hiding this comment.
One thing I think we should look into is how to make our reference documentation look better
The first 5 methods in https://googleapis.github.io/gax-php/main/Google/ApiCore/Options/ClientOptions.html are not helpful, the top code example does not really show how to use the class, and the first method that is useful (__construct) is a completely unformatted mess.
I'm not sure what impact it would have making the options public, and if that would help or hurt, but we should look into that also.
src/ClientOptionsTrait.php
Outdated
|
|
||
| // The constructor of ClientOptions construct creates a `TransportOptions`. | ||
| // We call `toArray` on it and nested items that uses the `optionsTrait` to keep | ||
| // the same behaviour as if it was just an array. |
There was a problem hiding this comment.
We could also make toArray call toArray on those items.... it would be a breaking change, so I think it's a bad idea. It's a shame we didn't do that before! Maybe in a 2.0 version...
There was a problem hiding this comment.
Coming back almost a year later - we could add an argument to toArray such as
public function toArray($recursive = false): arrayAnd then pass it down to the other arrays.
Although since this class is currently labeled @internal, we can make breaking changes if we want
src/Options/ClientOptions.php
Outdated
| use OptionsTrait; | ||
|
|
||
| private ?string $apiEndpoint; | ||
| public ?string $apiEndpoint; |
There was a problem hiding this comment.
The only concern I have here is that if there's some logic being done in the setters (which is the case for a few of these), it would be lost in setting these manually, which may lead to some confusing behavior.
|
@Hectorhammett I would suggest, just for safety, that we mark any changing methods as EDIT: Honorable mention here is to also move |
| $clientOptions->setApiEndpoint('TestEndpoint.com'); | ||
| $builtOptions = $client->buildClientOptions($clientOptions); | ||
|
|
||
| $this->assertEquals($clientOptions->apiEndpoint, $builtOptions['apiEndpoint']); |
There was a problem hiding this comment.
| $this->assertEquals($clientOptions->apiEndpoint, $builtOptions['apiEndpoint']); | |
| $this->assertEquals($clientOptions['apiEndpoint'], $builtOptions['apiEndpoint']); |
There was a problem hiding this comment.
I know that we are trying to keep things compatible and such, but it breaks my heard to use array notations vs $variable->property </3
|
Merged in #621 |
No description provided.