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

Incosistent items API in Content Module #200

Open
Tiefseetauchner opened this issue Jul 27, 2024 · 1 comment
Open

Incosistent items API in Content Module #200

Tiefseetauchner opened this issue Jul 27, 2024 · 1 comment

Comments

@Tiefseetauchner
Copy link
Contributor

Howdy! I noticed that the code in modules/Content/api.php is incosistent in the value it returns:

$items = $app->module('content')->items($model, $options, $process);
if (count($items)) {
    $app->trigger('content.api.items', [&$items, $model]);
}
if (isset($options['skip'], $options['limit'])) {
    return [
        'data' => $items,
        'meta' => [
            'total' => $app->module('content')->count($model, $options['filter'] ?? [])
        ]
    ];
}
return $items;

In case a skip and limit are set, it just completely changes the API. Dynamically. This makes it extraordinarily hard to use the pagination feature (which btw, I love that it's a thing in the first place), especially as this is entirely undocumented. The first time I learned that this is a behavior was when my app suddenly started crashing when I implemented pagination.

I would like to fix this. Either by documenting it in the openapi docs or by changing the interface (scary).

I'm gonna make a PR for the first option soon but I wanted to have a better place for discussion so if you say "Yes, let us change the interface" I'll make a new branch and new PR (as I'll use the prior option in my usecase for now and I don't need cutting edge currently)

@aheinze
Copy link
Collaborator

aheinze commented Jul 28, 2024

Thank you for providing a PR for OpenAPI. The reasoning behind it is based on discussions I had for the previous Cockpit version (<v2). People wanted a simplified response structure (just the content items).

But I also get your concerns. A solution could be an additional parameter to force a specific response data structure 🤔

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

No branches or pull requests

2 participants