jolicode / slack-php-api

:hash: PHP Slack Client based on the official OpenAPI specification
https://jolicode.github.io/slack-php-api/
MIT License
222 stars 56 forks source link

Introduce client decorator to handle collection #69

Closed pyrech closed 3 years ago

pyrech commented 4 years ago

Proposal

Here is a proposal to ease the usage of pagination for collection endpoint (only cursor based pagination are handled at the moment, but we could add classic pagination later).

I'm not a big fan of this solution because it's a bit too magical. Thus we loose everything related to typehint and IDE suggestion :confused: We could add some phpdoc on the decorator to define the methods but this would be a manual process (unless we find a way to automate it).

Here is a sample usage:

$client = ClientFactory::create('Your token');
$client = new CollectionClientDecorator($client);

/** @var ObjsUser[] $users */
$users = iterator_to_array($client->usersList([
    // Add some query parameters if needed
    // 'custom_config' => true,
]));

Alternative

If we do not accept this implementation, I think the only thing we can propose is something like this (a bit more verbose but it do keep some typehint):

$client = ClientFactory::create('Your token');

// $queryParameters contains the cursor and a default limit
$callback = function($queryParameters) use ($client) {
    // Add some query parameters if needed
    // $queryParameters['custom_config'] = true;
    $response = $client->usersList($queryParameters);
    return [
        'items' => $response->getMembers(),
        'cursor' => $response->getResponseMetadata() ? $response->getResponseMetadata()->getNextCursor() : '',
    ];
};
$collection = new CursorBasedCollection($callback);

/** @var ObjsUser[] $users */
$users = iterator_to_array($collection);

Ref #64

pyrech commented 4 years ago

I like your paginate method (even if it does not fix the problem with the typehints)... but the client class is still generated by Jane, so we can not edit it directly. I think, we should probably define a new child Client that will decorate the generated Client inside the ClientFactory. That means people not using the ClientFactory will not be able to use the paginate method.

damienalexandre commented 4 years ago

Yes we could event build our own class extending https://github.com/jolicode/slack-php-api/blob/master/generated/Client.php to add new methods like:

damienalexandre commented 4 years ago

How can we move forward with this feature?

The implementation looks good to me and to answer the last question: should we care about users that do not use our ClientFactory? That's an extreme edge case for power user, I don't think that's an issue.

damienalexandre commented 3 years ago

There is still some unresolved comment here (https://github.com/jolicode/slack-php-api/pull/69#discussion_r368608941), do you want to pursue with this PR?

pyrech commented 3 years ago

PR updated with a new Client (instead of the decorator) that provides an iterate() method and virtual iterateXXX() methods for each iterable endpoints. Here is the userland diff when retrieving all the users:

     $client = ClientFactory::create('Your token');

-    /** @var ObjsUser[] $users */
-    $users = [];
-    $cursor = '';
-
-    do {
-        // This method requires your token to have the scope "users:read"
-        $response = $client->usersList([
-            'limit' => 200,
-            'cursor' => $cursor,
-        ]);
-
-        $users = array_merge($users, $response->getMembers());
-        $cursor = $response->getResponseMetadata() ? $response->getResponseMetadata()->getNextCursor() : '';
-    } while (!empty($cursor));
+    /** @var ObjsUser[] $users */
+    $users = iterator_to_array($client->iterateUsersList());

PR is mergeable in its current state.

Next steps will be to:

pyrech commented 3 years ago

No BC break here, the new client inherit from the old one.

For the documentation, yes this totally deserves more explanation. I'm just not sure where to put that. In the readme (to ease discoverability)? In a dedicated "usage" documentation (to avoid "too much information in the readme")?

pyrech commented 3 years ago

Can we merge this PR as is? I would like to refactor the whole documentation in a dedicated PR