Add start_index and end_index #169
Conversation
| ]; | ||
| $this->assertEquals($expected, array_keys($rendered)); | ||
| $this->assertEquals(100, $rendered['total_items']); | ||
| $this->assertEquals(3, $rendered['page']); | ||
| $this->assertEquals(10, $rendered['page_count']); | ||
| $this->assertEquals(10, $rendered['page_size']); | ||
| $this->assertEquals(20, $rendered['start_index']); | ||
| $this->assertEquals(29, $rendered['end_index']); |
There was a problem hiding this comment.
While this correctly ensures that the newly introduced attributes are present, we also need tests covering what happens in terms of which items are in the collection when those attributes are present in the initial request.
Ideally, all of these, including the above assertions, should be in a separate tests to ensure that existing behavior remains unchanged, while the new behavior is also present.
I'd have tests demonstrating:
- what happens when both items are present in the request
- what happens when only the
start_indexis present in the request (for instance, how does this interact with pagination?) - what happens when only the
end_indexis present in the request (is this valid? if so, how does it interact with pagination?)
| @@ -605,6 +605,14 @@ public function renderCollection(Collection $halCollection) | |||
| $payload['page'] = ($payload['page_count'] > 0) | |||
| ? $halCollection->getPage() | |||
| : 0; | |||
| $payload['start_index'] = $payload['page'] > 0 | |||
| ? ($payload['page_size'] * ($payload['page'] - 1)?:1) | |||
There was a problem hiding this comment.
Add spaces around the ?: operator, please.
| ? ($payload['page'] < $payload['page_count'] | ||
| ? (($payload['page_size'] * $payload['page']) - 1) | ||
| : $payload['total_items']) | ||
| : 0; |
There was a problem hiding this comment.
This is pretty convoluted and hard to follow. Maybe write a dedicated method for calculating the value? That way you can return early when specific conditions are met.
There was a problem hiding this comment.
Do you think it should be better to get values from Paginator instead of Hal plugin?
There was a problem hiding this comment.
IIRC, we use the values in the zf-hal configuration to seed the paginator. That said, the total number of items and number of pages is generally from the paginator, so getting those from that would likely be the best idea.
|
Should such logic not be a front end implementation? |
|
This repository has been closed and moved to laminas-api-tools/api-tools-hal; a new issue has been opened at laminas-api-tools/api-tools-hal#4. |
|
This repository has been moved to laminas-api-tools/api-tools-hal. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
So you can show : Item [start_index] to [end_index] on [total_items]
or if you prefer : Item 20 to 29 on 100