Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Add start_index and end_index #169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add start_index and end_index #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Plugin/Hal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add spaces around the ?: operator, please.

: 0;
$payload['end_index'] = $payload['page'] > 0
? ($payload['page'] < $payload['page_count']
? (($payload['page_size'] * $payload['page']) - 1)
: $payload['total_items'])
: 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@e-belair e-belair Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it should be better to get values from Paginator instead of Hal plugin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} elseif (is_array($collection) || $collection instanceof Countable) {
$payload['total_items'] = isset($payload['total_items']) ? $payload['total_items'] : count($collection);
}
Expand Down
4 changes: 4 additions & 0 deletions test/Plugin/HalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,12 +1010,16 @@ public function testRenderingPaginatorCollectionRendersPaginationAttributes()
'page_size',
'total_items',
'page',
'start_index',
'end_index',
];
$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']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_index is present in the request (for instance, how does this interact with pagination?)
  • what happens when only the end_index is present in the request (is this valid? if so, how does it interact with pagination?)

return $rendered;
}

Expand Down