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

Fix collection retrieval #28

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

Conversation

nbusseneau
Copy link
Contributor

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

@nbusseneau
Copy link
Contributor Author

I noticed this while trying to use the archive plugin with an @self.descendants collection :)

@mahagr mahagr requested a review from rhukster October 9, 2020 07:28
@mahagr
Copy link
Member

mahagr commented Oct 9, 2020

@rhukster Can you take a look?

@rhukster rhukster marked this pull request as draft October 11, 2020 20:55
archives.php Outdated
@@ -224,9 +224,9 @@ protected function getArchives($filters, $operator, $order)
}
}
if ($new_approach) {
$collection = $page->children();
$collection = $page->collection();
Copy link
Member

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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
@nbusseneau nbusseneau marked this pull request as ready for review October 26, 2020 21:09
@nbusseneau
Copy link
Contributor Author

Bumping this -- don't hesitate to HMU if I can do anything.

@nbusseneau
Copy link
Contributor Author

@rhukster Revisiting old PRs -- is there anything I can do to get this across the finish line?

@rhukster
Copy link
Member

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.

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.

3 participants