Fix #352 - [PHP] [Improvement] Make Api return object instead of array

[thomasphansen]

PR checklist

• Read the contribution guidelines.

• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.

• Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.

• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

• @jebentier

• @dkarlovi

• @mandrean

• @jfastnacht

• @ackintosh

Description of the PR

This PR will:

• add the class ApiResponse to the client
• implement *WithApiResponse methods in the Api classes, returning ApiResponse
• to preserve compatilibility, keep the *WithHttpInfo methods

For more details, please check issue #352.

[wing328]

@thomasphansen thanks for the PR. From what I understand, this is not a breaking change as you're simply introducing another method.

Do you know if there'a any PHP guideline or best practice in terms of what's more preferrable to be returned by method: object vs array?

I'm not against the idea of using ApiResonse, which is what we use for Java API client for example, and would like to know which way (object or array) is more common among PHP developers.

[thomasphansen]

Hello William, sorry for the late reply!

There are some articles on the internet where you may find general guidelines - even though this is actually a matter of coding OOP or not. Here goes what I found:

• https://softwareengineering.stackexchange.com/questions/296752/php-when-to-use-arrays-and-when-to-use-objects-for-mostly-data-storing-code-co (the first answer is by far the most clear and straight-forward answer for this object vs array discussion).
• https://www.brandonsavage.net/stop-returning-arrays-use-objects-instead/
• https://stackoverflow.com/questions/6710967/why-return-object-instead-of-array (look at the two first answers, the first one offers a list of articles talking about the subject, and the second one is pretty detailed on why it is better to use objects instead of arrays).

As a matter of fact, I couldn't find any article defending the use of arrays instead of objects, except of course for not structured data (which is not the case). The first article has actually a very nice argument for using objects in our case:

Are you building a library or shared code base?
If so, use a class! Think about some new developer who is using your library, or another developer somewhere else in the company who has never used your code before. It will be much easier for them to look at a class definition and understand what that class does, or see a number of functions in your library which accept objects of a certain class, than to try and guess what structure they need to generate in a number of arrays. Also, this touches on the data consistency issues with #1: if you're developing a library or shared code base, be nice to your users: give them classes which enforce data consistency and protect them from making errors with the design of your data.

Hope it helps! :)

I just noticed you have added the PR on return types to master - I'll rebase my PR as soon as I have time, ok? :)

[ackintosh]

@thomasphansen Thanks for the suggestion!
I think object is not just a container, it has a responsibility. What do you think is the responsibility of our ApiResponse?

[thomasphansen]

Hello Akihito,

Thanks for your asking! :)

IMO, I don't think this is the case: REST and SOAP interfaces should return DTO's, which by definition have no logic associated to them - their only responsibility is to deliver the data obtained from the service.

Also, just remember that all the generated Model classes also follow DTO pattern, since their only responsibility (like ApiResponse) is to contain data, with no logic involved... :)

(DTO pattern need is described by Martin Fowler here :) )

Hope it makes sense! :)

[ackintosh]

Sorry for late reply! 💦

https://martinfowler.com/eaaCatalog/dataTransferObject.html

There are 2 points I think important in the article.

• Although the main reason for using a Data Transfer Object is to batch up what would be multiple remote calls into a single call,

• it's worth mentioning that another advantage is to encapsulate the serialization mechanism for transferring data over the wire.

That describes the DTO's responsiblity. 💡

Take the generated Model classes for example: Model has serialization and validation mechanism (and the logic to execute its responsiblity). And these are used in the process transfering data (e.g. Pet) between client and server.

Array can't alternative that as Array is just container.

I can't find the reason we should use Class instead of Array in the return value yet. 🤔💦

[wing328]

@thomasphansen @ackintosh the way I can see moving this forward is to add an option for the PHP client generator (e.g. returnObject) so that users can enable this option if needed.

[thomasphansen]

If you all agree with that, then I can code the change to make it an option! :)

[wing328]

@thomasphansen quick question: have you evaluated other SDKs such as Facebook, Twitter, Slack, etc to see if these PHP SDK return a list or object?

This is not to say these SDKs should be our reference so good to know if they follow a particular pattern when returning data from functions.

[wing328]

@thomasphansen if you've time for a quick chat, please ping me via https://gitter.im. Thanks.

[thomasphansen]

Hello @wing328

Sorry for the late reply (too busy here! :) ).

William, I am really sorry, but I don't have time to go through other API's and do such a check. The main reason for this pull request was (and it is) to make it easier to work with, and to make it offer an interface that would be coherent to what you already offer in other generators (such as the Java one, as you mentioned before). And even though I really love discussions on subjects like what a perfect API interface model would be, I also see that such a discussion should then affect all the current generators, not only the PHP one. IMHO, out of a major refactoring scenario, it is a bit pointless to argue if the APIResponse concept is valid or not, since it is already present in the openapi project.

Which means: if openapi is not enforcing a standard interface model through the many generators you have, then each generator is, in last analysis, "free" to do what they want - and thus, accepting or not this push request is actually more about if the php generator team agrees or not with it. And I am perfectly fine with that! You are the ones taking care of the code, and it should be your decision if a PR is a valid contribution or not. And if someone does not agree, well, that's what forks were created for! :)

For now, in our own projects, I'm using the fork we did, and we will do our best to keep it up to date with your master branch. If you decide that this contribution is relevant, or if you think there is anything I could do to make it more adequate to openapi, just let me know, ok? :)

Have a nice day! :)

[ybelenko]

@thomasphansen
Likely we don't need to reinvent anything, we already have PSR-7 standard which describes HTTP Response perfectly.

Both Psr\Http\Message\RequestInterface and Psr\Http\Message\ResponseInterface extend
Psr\Http\Message\MessageInterface. While Psr\Http\Message\MessageInterface MAY be implemented directly, implementors SHOULD implement Psr\Http\Message\RequestInterface and Psr\Http\Message\ResponseInterface.

Following to PSR-7 standard Response class must implement methods from MessageInterface:

• getProtocolVersion
• withProtocolVersion
• getHeaders
• hasHeader
• getHeader
• getHeaderLine
• withHeader
• withAddedHeader
• withoutHeader
• getBody
• withBody

and from ResponseInterface

• getStatusCode
• withStatus
• getReasonPhrase

How to resolve:

1. Add psr/http-message package to Composer dependencies
2. Update ApiResponse class to implement ResponseInterface
3. Implement required methods from ResponseInterface and MessageInterface
4. Return ResponseInterface instance instead of array in synchronous api calls

As example you can check how Slim library handles this problem in their Response class.

It obviously is a big change, but we can split this work into bunch of PRs and return null in methods which are not critical for now.

❗ Update:

I've noticed that guzzlehttp/guzzle 6.x already follows PSR-7, maybe we can extend GuzzleHttp\Psr7\Response and not write own ApiResponse class from scratch.

[thomasphansen]

@ybelenko IMHO this is the perfect solution, since it implements a PSR and extends a well-known, established and used library (guzzle). @wing328 and @ackintosh what do you think about it? :)

[ybelenko]

@thomasphansen
One day after I'm not sure that I've suggested good solution. Let's take a look at current code:

/**
 * Operation fakeOuterNumberSerialize
 *
 * @param  float $body Input number as post body (optional)
 *
 * @throws \OpenAPI\Client\ApiException on non-2xx response
 * @throws \InvalidArgumentException
 * @return float
 */
public function fakeOuterNumberSerialize($body = null)

and with a quick look you perfectly knows what you'll recieve and how to extract data(don't need to extract anything at all)
Let's imagine with PSR-7 response instead:

/**
 * Operation fakeOuterNumberSerialize
 *
 * @param  float $body Input number as post body (optional)
 *
 * @throws \OpenAPI\Client\ApiException on non-2xx response
 * @throws \InvalidArgumentException
 * @return \Psr\Http\Message\ResponseInterface
 */
public function fakeOuterNumberSerialize($body = null)

now you know that you'll receive ResponseInterface and no clue how to extract float data that you looking for.

It seems to me that we need to generate something like ApiResponseForFakeOuterNumberSerialize which implements PSR-7 interface and have getters and setters. If so user can see that he'll recieve this class as a return and he can read setters and getters from it's documentation to get data. I'm afraid that it's a huge extra work.

[wing328]

William, I am really sorry, but I don't have time to go through other API's and do such a check. The main reason for this pull request was (and it is) to make it easier to work with, and to make it offer an interface that would be coherent to what you already offer in other generators (such as the Java one, as you mentioned before). And even though I really love discussions on subjects like what a perfect API interface model would be, I also see that such a discussion should then affect all the current generators, not only the PHP one. IMHO, out of a major refactoring scenario, it is a bit pointless to argue if the APIResponse concept is valid or not, since it is already present in the openapi project.

@thomasphansen that's perfectly fine. To be clear I'm not trying to argue if APIResponse is valid or not. I just want to know which way is more preferable for PHP developers.

Which means: if openapi is not enforcing a standard interface model through the many generators you have, then each generator is, in last analysis, "free" to do what they want - and thus, accepting or not this push request is actually more about if the php generator team agrees or not with it. And I am perfectly fine with that! You are the ones taking care of the code, and it should be your decision if a PR is a valid contribution or not. And if someone does not agree, well, that's what forks were created for! :)

At the very beginning, we tried to enforce a standard so that all API clients behave more or less the same way (e.g. all clients should return an object). However, we got feedback later what make senses in one generator (e.g. Java API client) may not welcome by developers using other languages (e.g. Perl). As a result, we let the users of the generator decide what works for them. Sometime we may not get a strict answer and we will need to provide an option to make both parties having different opinions happy.

In this particular case, I can't speak on behalf of all PHP developers. Based on my guess, some may prefer returning a list while others may prefer returning an object (e.g. ApiResponse). I still think providing an option is the best middle ground to make everyone happy.