-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix collection retrieval #28
base: develop
Are you sure you want to change the base?
Conversation
I noticed this while trying to use the archive plugin with an |
@rhukster Can you take a look? |
archives.php
Outdated
@@ -224,9 +224,9 @@ protected function getArchives($filters, $operator, $order) | |||
} | |||
} | |||
if ($new_approach) { | |||
$collection = $page->children(); | |||
$collection = $page->collection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collection() alone is not enough. pagination in the collection definition will cause only the first page of results to be taken into account for the archives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, I'll revisit this PR when I have some free time (probably next week) and check if disabling pagination works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabled pagination in latest version, seems to be working as intended 👌
`children()` only retrieves direct children, as per the code: https://github.com/getgrav/grav/blob/1.6.27/system/src/Grav/Common/Page/Page.php#L2422 `collection()` is the proper retrieval method: https://github.com/getgrav/grav/blob/1.6.27/system/src/Grav/Common/Page/Page.php#L2675 This can be confirmed by looking at what's used for collection operations (e.g. `isFirst()` or `isLast()`): https://github.com/getgrav/grav/blob/1.6.27/system/src/Grav/Common/Page/Page.php#L2439
b881279
to
b6add10
Compare
Bumping this -- don't hesitate to HMU if I can do anything. |
@rhukster Revisiting old PRs -- is there anything I can do to get this across the finish line? |
I have this issue in a list to review when i get a chance. Need to test to ensure backwards compatibility as it is changing a behavior that users might rely on. |
children()
only retrieves direct children, as per the code:https://github.com/getgrav/grav/blob/1.6.27/system/src/Grav/Common/Page/Page.php#L2422
collection()
is the proper retrieval method:https://github.com/getgrav/grav/blob/1.6.27/system/src/Grav/Common/Page/Page.php#L2675
This can be confirmed by looking at what's used in default Quark theme when retrieving collection blog page template:
https://github.com/getgrav/grav-theme-quark/blob/2.0.3/templates/blog.html.twig#L3