Skip to content

Fix collection retrieval#28

Open
nbusseneau wants to merge 1 commit into
getgrav:developfrom
nbusseneau:bugfix/collection-items
Open

Fix collection retrieval#28
nbusseneau wants to merge 1 commit into
getgrav:developfrom
nbusseneau:bugfix/collection-items

Conversation

@nbusseneau

Copy link
Copy Markdown
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
Copy Markdown
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

mahagr commented Oct 9, 2020

Copy link
Copy Markdown
Member

@rhukster Can you take a look?

@rhukster rhukster marked this pull request as draft October 11, 2020 20:55
Comment thread archives.php Outdated
}
if ($new_approach) {
$collection = $page->children();
$collection = $page->collection();

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 force-pushed the bugfix/collection-items branch from b881279 to b6add10 Compare October 26, 2020 21:08
@nbusseneau nbusseneau marked this pull request as ready for review October 26, 2020 21:09
@nbusseneau

Copy link
Copy Markdown
Contributor Author

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

@nbusseneau

Copy link
Copy Markdown
Contributor Author

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

@rhukster

Copy link
Copy Markdown
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