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

Fixed Query::paginate() #14

Merged
merged 1 commit into from
Oct 2, 2020
Merged

Fixed Query::paginate() #14

merged 1 commit into from
Oct 2, 2020

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants