Skip to content

Conversation

Max13
Copy link
Contributor

@Max13 Max13 commented Oct 2, 2020

I don't understand the original intention of the array of value true in the paginate() method, so I assumed they are placeholders for when Query::get() doesn't output messages (in case of a bug maybe).

So I kept this behavior, while fixing #13 which I think is a bug.

If I misunderstood the behavior, please explain, I'll revert and fix my case.

@Webklex
Copy link
Owner

Webklex commented Oct 2, 2020

Hi @Max13 ,
you are right. It looks like I had a bit of a brain failure ;)

The required total got already set in Query::get() and therefor the returned collection is ready to be used as a paginator.

I somehow thought I had to pad the collection, but I did a mistake during testing - that's how we ended up here.

I think its save to remove the true collection padding since it is not required at all (?). I don't think a potential error should be camouflaged like this. So how about something like this?

public function paginate($per_page = 5, $page = null, $page_name = 'imap_page'){ if ($page === null && isset($_GET[$page_name])) { $this->page = $_GET[$page_name] > 0 ? intval($_GET[$page_name]) : 0; } elseif ($page > 0) { $this->page = $page; } $this->limit = $per_page; return $this->get()->paginate($per_page, $this->page, $page_name); }

Do you want to update your PR or should I merge it and do the changes myself? :)

Thanks for taking your time and willingness to contribute. I very much appreciate it 👍

if (
$page === null
&& isset($_GET[$page_name])
&& $_GET[$page_name] > 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think this condition is necessary (instead of ternary condition) because if $_GET[$page_name] isn't positive, Query::$page shouldn't be touched and should remain to it's default value: 1

Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't $_GET[$page_name] throw a notice if it isn't set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If $_GET[$page_name] isn't set, a notice should be raised.

L:256 checks if $_GET[$page_name] is set. If not, L:257 won't be checked, so it would jump the block and try the next condition (hence, without touching $this->page which would remain to 1)

@Max13
Copy link
Contributor Author

Max13 commented Oct 2, 2020

Brain fails happen, I hope removing the unnecessary pad won't break anything in existing code.

I forced-push the changes we agree on, and added comments.

You're welcome, it's for the greater good and... me 😁

@Webklex Webklex merged commit 12dd9ca into Webklex:master Oct 2, 2020
@Max13 Max13 deleted the paginate branch October 2, 2020 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants