Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

PHP - pass array(key-value pair) as an argument instead of comma separated individual arguments #641

Closed
rajanmodi opened this issue Jul 25, 2018 · 12 comments

Comments

@rajanmodi
Copy link

rajanmodi commented Jul 25, 2018

Description

Functions generated automatically in SDK should support array(key-value pair) as an argument.

Suppose there is search function with 66 arguments.
search($this->arg1 = null, $this->arg2 = null, ...., $this->arg66 = null)

When we need to make a call to this search function ,we need to provide all the arguments
$apiInstance->search($this->arg1 = 5, $this->arg2 = 10, $this->arg3 = 20,.....,$this->arg66 = 1)

What if I only want to pass only two arguments
$apiInstance->search($this->arg1 = 5,$this->arg31 = 95).
now this will assign 5 to arg1 and 95 to arg2. I need to assign value of 95 to arg31

I think solution will be generating function argument as an array with key-value pair for such cases .

Instead of keeping comma separated arguments, array should be there..

openapi-generator version
OpenAPI declaration file content or url
Command line used for generation
Steps to reproduce
Related issues/PRs
Suggest a fix/enhancement
@wing328
Copy link
Member

wing328 commented Jul 25, 2018

@rajanmodi thanks for the suggestion.

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh @ybelenko

@ybelenko
Copy link
Contributor

@rajanmodi Hmm... Give us an example? I can't find any function with more than one argument in current PHPClient samples. We're talking about models, right?

/**
 * Constructor
 *
 * @param mixed[] $data Associated array of property values
 *                      initializing the model
 */
public function __construct(array $data = null)
{
    $this->container['bill_id'] = isset($data['bill_id']) ? $data['bill_id'] : null;
    $this->container['bill_date'] = isset($data['bill_date']) ? $data['bill_date'] : null;
    $this->container['charges'] = isset($data['charges']) ? $data['charges'] : null;
    $this->container['charge_desc'] = isset($data['charge_desc']) ? $data['charge_desc'] : null;
}

@wing328
Copy link
Member

wing328 commented Jul 25, 2018

@ybelenko I think @rajanmodi is referring to method arguments, e.g. updatePetWithForm with 3 parameters only and let's imagine it has 20 additional optional parameters.

@ybelenko
Copy link
Contributor

@wing328 If so, I think that more common practice is to create endpoints with just few parameters, or with one complex JSON object parameter.
I like current "full list of parameters" appouch more and user always can rewrite mustache template a bit.

But key-values pairs very handy when you add both required and optional parameters and rebuild client SDK. You can use fresh SDK right away without need to rearrange arguments in functional call.

In total, 1 pro and 1 con from me 😄

@rajanmodi
Copy link
Author

rajanmodi commented Jul 26, 2018 via email

@ybelenko
Copy link
Contributor

@rajanmodi
66 arguments in search operation, huh?
Even Facebook and Twitter doesn't have API methods with huge arguments list like that.
Anyway, I understand why you don't wanna use object as input parameter, obviosly your HTTP method is GET for a search. Maybe you can use enum filters to make arguments list shorter, in way:

/jobs/?status_filter=closed|canceled|abandoned

@rajanmodi Can you give us endpoint example, when you need huge stack of input parameters?
Another suggestion to solve this problem, we can extend PHPClient config with useArrayAsArgument which will be false by default.

@rajanmodi
Copy link
Author

rajanmodi commented Jul 26, 2018 via email

@ackintosh
Copy link
Contributor

ackintosh commented Jul 27, 2018

Ideally, the issue should be resolved with named parameters but PHP doesn't have that mechanism. 🤔

How about implementing the wrapper class for the search method like below?

<?php
// You only have to pass the necessary arguments
WrapperForExampleApi::search(['arg2' => 'x', 'arg4' => 'x']);

/**
 * hand-written wrapper class for ExampleApi
 */
class WrapperForExampleApi
{
    public static function search()
    {
        $args = self::defaultArguments();
        $passed = func_get_args()[0];
        $merged = array_merge($args, $passed);

        return call_user_func_array([new ExampleApi, 'search'], $merged);
    }

    private static function defaultArguments()
    {
        $exampleApi = new ExampleApi;
        $method = new \ReflectionMethod($exampleApi, 'search');

        $arguments = [];
        foreach ($method->getParameters() as $parameter) {
            $arguments[$parameter->getName()] = $parameter->getDefaultValue();
        }

        return $arguments;
    }
}


/**
 * auto-generated api class
 */
class ExampleApi
{
    public function search($arg1 = null, $arg2 = null, $arg3 = null, $arg4 = null)
    {
        var_dump('$arg1 = ' . $arg1);
        var_dump('$arg2 = ' . $arg2);
        var_dump('$arg3 = ' . $arg3);
        var_dump('$arg4 = ' . $arg4);
    }
}

@rajanmodi
Copy link
Author

This solution will work, but I am wandering if there is anything like java adapter classes in php.
Automatic sdk generation, if supports this java adapter class concept, then this will be great help and this problem will be solved and great rescue for devs.

@ackintosh
Copy link
Contributor

(This is just my personal opinion 😉)

I think that adopting the adapter class workaround to SDK generation should avoid as much as possible.

because...

  • the adapter class is workaround
  • ideally the issue should be resolved with named parameters
  • adopting the workaround is easy but removing it is hard as breaking change takes pains to SDK's users.

So I suggest SDK users who has the issue like you reported implement a adapter class like #641 (comment) .

@ackintosh
Copy link
Contributor

ackintosh commented Dec 20, 2018

@jeandonaldroselin
Copy link

jeandonaldroselin commented Mar 30, 2024

@rajanmodi The group parameter support in PHP client has been implemented by @wing328 and released as v3.3.2. Please give it a try. 😉

Samples

  • Spec

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml#L774

  • Generated code

https://github.com/OpenAPITools/openapi-generator/blob/master/samples/openapi3/client/petstore/php/OpenAPIClient-php/lib/Api/FakeApi.php#L2538-L2541

Hello @ackintosh,

First thank you for your great contributions.

I have adopted your tools for a while now but I face the same issue as @rajanmodi.

The consequences of that are multiple :

  • I waste time when writing the methods calling the PHP client methods (I need to provide all the arguments, in some cases 40 arguments that's a normal use case for searching entities)

  • I do a lot of mistake after updating my PHP client (if I have a parameter which have been added, some method won't work anymore if I forgot to update them)

I could adopt the given workaround #641 (comment), but I want to keep my projects the most simple as possible because I want to be able to integrate juniors in the project as well. So the more I use standard they can fin in documentation, the best it is.

I don't find any documentation on "how to generate methods supporting array instead of separate arguments as parameters".

Please can you help me ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants