Improve request configuration pattern

[Ndiritu]

Challenge: It can be difficult to know which request configuration object and query parameter object to import. It also doesn't feel as fluent to type.

Developer realises that they can configure the GET request

Developer creates config object but has to figure out which object to import

Developer goes through the same when creating the query parameter object

Final code

In v1, the developer would add query parameters to the URL directly and pass headers:

$response = $graphClient->createRequest('GET', '/users/user-id/messages?$select=subject,body&$top=10')
                                          ->addHeaders(['Prefer' => 'outlook.body-content-type=text'])
                                          ->execute();

Perhaps our initial Kiota design would be more fluent?

$response = $graphServiceClient->usersById('user-id')->messages()->select(['subject', 'body'])->top(10)->get(['Prefer' => 'outlook.body-content-type=text']);

[isvargasmsft]

Awesome, thank you very much for this explanation Philip.

[sebastienlevert]

The experience is very similar to what we had with JavaScript / TypeScript. Here are my thoughts :

• This is not an enjoyable experience for developers. It's confusing and honestly, unnecessary.
• I understand that each "*GetRequestConfiguration" is "unique" as it adds the features of Graph (support of select, top, etc. based on the underlying endpoint and the OpenAPI description). It explains it, but I don't think it justifies this complexity.
• The fluent style approach is great and simplifies the discoverability / usage of the API. Just the config object makes it very cluttered.

My thoughts (with my limited knowledge of PHP) would lead to use an anonymous object. Can we do something like this in PHP?

$response = $graphServiceClient->usersById('user-id')->messages()->get({
	'queryParameters' => {
		'top' => 2,
	    'select' => ['subject', 'body']
	},
	'headers' => [
		'Prefer' => 'outlook.body-content-type=text'
	]
});

Very pseudo-code-y, but is there a way we can use this structure? This would remove the need to explicitly import. Thoughts?

[36864]

My thoughts (with my limited knowledge of PHP) would lead to use an anonymous object. Can we do something like this in PHP?

In PHP you'd probably use associative arrays for this. Pretty much the same syntax you described, except with square brackets instead of curly brackets.

There is another pattern that I see often in which the developer passes a callable as a parameter to a function, and that callable receives the appropriate configuration object. Something like the following:

$response = $graphServiceClient->usersById('user-id')->messages()->get(function($config) {
    $config->top = 2;
    $config->select = ['subject', 'body'];
    $config->headers = [ 'Prefer' => 'outlook.body-content-type=text'];
});

These approaches don't have to be mutually exclusive. There's no concept of "overloading" in PHP, but it can be emulated by inspecting the types of received arguments.

[Ndiritu]

Thanks for the feedback & suggestions @36864. Will work on this early next month.

[Ndiritu]

@baywet @isvargasmsft @shemogumbe @SilasKenneth any thoughts?

[baywet]

Kiota doesn't have any prescriptive style for the request configuration parameters:

• Dotnet uses a callback with optional parameters.
• Java uses callback parameters with overloads
• Go uses simple parameters without overloads.
• JS/TS has ongoing work to figure out the best combo (probably optional parameters, with JSON notation for the request body).

If PHP wants to use a callback for the request configuration (options, query parameters, headers), go for it (that's a decision for Isaac to make as a PM). Keep in mind two things:

• The parameters format should be the same across the experience (executor methods, generator methods, batching...)
• We'll want to choose a single parameters experience to avoid duplicating efforts on our end for now.

[Ndiritu]

(Sorry to pick up this conversation after a long time)

The callback approach and anonymous object/dictionary approach would require a developer to first figure out what query parameter options are supported per endpoint.

From discussions with @isvargasmsft, generating fluent methods for configuration seems like a better developer experience i.e. generate a withHeaders(array $args)/headers(array $args), withOptions(array $options), select(array $args), expand(array $args) ...

SDK size shouldn't be a significant concern with this approach since the methods will be defined once per request builder vs currently we declare a request configuration class for each HTTP method & a query parameter class per request builder.

Thoughts? @sebastienlevert @baywet @SilasKenneth @shemogumbe

[baywet]

To recap the discussion on the meeting. $graph->users()->select()->top()->headers()->get(); would not work since query parameters vary by operation/verb.

We could do $graph->users()->get()->select()->top()->headers()->execute(); but that'd introduce a new intermediate request object for each operation (SDK size) and force people to call execute even when they don't want to configure the request. We've had lots of feedback in dotnet and Java that people were tired of having to call "buildRequest()" and this is why we did away with it during the early days of Kiota.

$response = $graphServiceClient->usersById('user-id')->messages()->get(function($config) {
    $config->top = 2;
    $config->select = ['subject', 'body'];
    $config->headers = [ 'Prefer' => 'outlook.body-content-type=text'];
});

is challenging since most IDEs wouldn't give you auto-completion for anything under $config, people would need to annotate the type in the callback definition which in terms of experience would give us the same as the no callback option.

@SilasKenneth made a suggestion to use arrays with PHP docs annotations instead which most IDEs would give a better support for.

@Ndiritu will try it so we have a basis for comparison between the current situation, callbacks, and arrays.

[Ndiritu]

PHPDoc doesn't have a way to define the "shape" of an array. However, static analysis tools (PHPStan, Psalm) and PHPStorm (Jetbrains paid PHP IDE) seem to have agreed on a standard for this:

/**
 * @param array{headers: array<string, string>, top: int, count: true, requestOptions: array<string, RequestOption>} $config
 */
function post(array $config) {}

(Auto-completion experience on PHPStorm)

VsCode doesn't have native auto-completion support & the most popular auto-completion extension(Intelephense) are yet to implement this.

I'd argue that we could still use arrays since it's a common config pattern in PHP e.g. Guzzle client.

Developers without auto-completion support from PHPStorm can rely on static analysis tools to catch errors before runtime.

Also, should we nest the query parameters under a queryParameters key or not?

[baywet]

Thanks for doing the investigation work.

Do we have publicly available data that states what IDEs PHP devs are generally using? (back in my days it was notepad++ with a few plugins, but hopefully that's not the case anymore 😉 ) (I just want to make sure we're not making a decision that's going to alienate a big percentage of our audience)

Also, should we nest the query parameters under a queryParameters key or not?

Yes, it's only a matter of time before somebody has a query parameter named headers or options and everything goes off the rails.

[Ndiritu]

Couldn't find any data other than JetBrains' report which is biased. I would guess VS Code has a strong user base seeing that it comes second.

Thanks for clarifying on the query params key.

[baywet]

Thanks for sharing this. 1% are still using notepad++ 🤣 Another interesting aspect of this survey is the fact that the vast majority of developers are not using static analysis, so we can't bank on that either (c.f. your previous comment).

So, to recap, auto-completion story for PHP request configuration is poor at best right now. Switching to array + this specific format of PHP docs would make it better for some people (PHP storm, the few using static analysis tools, potentially vs code users assuming it gets implemented), but would it make it worse for others?

[Ndiritu]

Took a fresh look at this. I think an ideal experience should feel intuitive/fluid and reduce the chances of errors.

Using the array format (esp if you're not using PHPStorm) has poor auto-completion which may lead to typos when setting array keys. Devs can only discover these errors during runtime. Also, they'll have to reference the function docs to know which query params are enabled etc.

I'd recommend sticking to the request config classes because:

1. We could improve the request config class experience by adding constructors arguments and leveraging PHP 8's Named Arguments.

This would prevent having to create the object before the executing the request e.g.

$config = new MessagesRequestBuilderGetRequestConfiguration();
$config->queryParameters = new MessagesRequestBuilderGetQueryParameters();
$config->queryParameters->top = 10;

$messages = $client->usersById($userId)->messages()->get($config)->wait();

And allow us to initialise the object inline while leveraging PHP 8's named arguments

$messages = $graphServiceClient->usersById(USER_ID)->messages()->get(new MessagesRequestBuilderGetRequestConfiguration(
        queryParameters: new MessagesRequestBuilderGetQueryParameters(
            select: [],
            top: 10
        )
    ));

2. The auto-completion experience still requires a dev to figure out the right class to import BUT alerts the developer of an error immediately:

• discovering you can configure the request

• importing the right request config class (Some pain)

• using named arguments to only set query parameters

• importing the right query parameters class (Some pain)

• setting query param values via named arguments leveraging autocompletion

It may be worth noting that the suggested imports in PHPStorm are more accurate to the desired classes. Hopefully with time the PHP VsCode plugin is improved.

PHPStorm auto-completion

But in case the wrong class is imported, the developer is aware immediately:

[Ndiritu]

What do you think @baywet @SilasKenneth @isvargasmsft @shemogumbe ?

[SilasKenneth]

Thanks @Ndiritu for providing the detailed approaches to getting a better request configuration experience. I feel confident having the constructors will help a lot with improving the developer experience, this should be very helpful for people using PHP < 8.0. Users with PHP >= 8 will have the added advantage of named arguments.

What do you think about having a method in the request config class that helps with creating a QueryParameters object for us so we don't have to know the right class for the query parameter or even import it?

Example

---

$config = new MessagesRequestBuilderGetRequestConfiguration(
         options: [$option],
         headers: ['Header-Name' => 'HeaderValue'],
         queryParameters: MessagesRequestBuilderGetRequestConfiguration::newQueryParameters(
                   select: ['content', 'subject']
              )
         );

[Ndiritu]

@SilasKenneth I like that idea! Less room for errors.

[baywet]

I like the approach, thanks for sharing it. A couple of questions/remarks:

Could you share how one defines those new constructors?

The static method under the request configuration type is a nice shorthand, it'll add to the weight of the overall SDK and we should keep that in mind.

[Ndiritu]

@baywet the constructors... PHP 8 promotes constructor arguments to class properties. Ordinary constructors would still work

class UsersRequestBuilderGetRequestConfiguration 
    {
        public function __construct(
            public ?array $headers = null,
            public ?array $options = null,
            public ?UsersRequestBuilderGetQueryParameters $queryParameters = null
        ) {
            $this->headers = $headers;
            $this->options = $options;
            $this->queryParameters = $queryParameters;
        }
    }

[Ndiritu]

Regarding increase in size, I would argue it's a cost we can pay to make the developer experience better in this case. However, I'll measure the size impact of having the static method and share. If it's "too much" we can re-consider it.

[baywet]

Thanks for sharing this. Do you need to assign the parameter to the property considering the parameter is an auto-property as far as I can tell? Would we even still need to declare the properties? (this would effectively mitigate some of the size increase but we should consider the impact on doc comments).

(so just this )

class UsersRequestBuilderGetRequestConfiguration 
    {
        public function __construct(
            public ?array $headers = null,
            public ?array $options = null,
            public ?UsersRequestBuilderGetQueryParameters $queryParameters = null
        ) {
        }
    }

[Ndiritu]

You're right actually. No need to do the assignments. We wouldn't need to declare properties and doc comments would still be available as far as I know.

/**
 * Undocumented function
 *
 * @param array|null $headers
 * @param array|null $options
 * @param UsersRequestBuilderGetQueryParameters|null $queryParameters
 */
public function __construct(
    public ?array $headers = null,
    public ?array $options = null,
    public ?UsersRequestBuilderGetQueryParameters $queryParameters = null
) {}

[SilasKenneth]

For the impact of static methods on size, I think it is going to be there but not that big since we only add the static method to Configuration classes with the QueryParameters property.