Add possibility to set pagination options on grids #68

vvasiloi · 2020-10-18T12:14:32Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	fixes Sylius/Sylius#10343
License	MIT

A similar solution can be applied to the Resource Bundle Sylius/SyliusResourceBundle#148.

The solution is based on this experiment: https://github.com/vvasiloi/doctrine-pagination

TODO:

test setting default values by parameters
add parameters for setting default values
test setting the values per grid
allow setting the values per grid
test with fetch join enabled/disabled per grid
test with fetch join enabled/disabled by default
test with order by to-many association field and allow output walkers enabled/disabled per grid
test with order by to-many association field and allow output walkers enabled/disabled by default
update documentation

Additional:

apply a similar solution to Sylius Resource Bundle
enable fetch join and output walkers in Sylius Standard
use configuration instead of parameters?

vvasiloi · 2020-10-18T12:17:58Z

@lchrusciel can I have a review, please? I need to know if and how I should move forward with this.

lchrusciel

Hey Victor,

this issue is with us for way too long. Your proposal seems to be in a good direction. However generic setting could be done with configuration component indeed. I would like to see it merged in SyliusGridBundle.

/cc @pamil, what is your opinion?

src/Bundle/Doctrine/ORM/DataSource.php

pamil

Thanks a lot and sorry for the delay! I've just rebased the PR to the newest master so that CI does not fail anymore. Also resolved @lchrusciel comment and added a comment not to rely on these parameters for defining global behaviour for now.

pamil · 2020-12-02T18:50:37Z

I think in the current form it's pretty good to go. I'd say the next steps, for now, would be "test with order by to-many association field and allow output walkers enabled/disabled per grid" and "update documentation".

Then it should be good to merge, we can skip configurable default settings for all grids for now and address that later. If so, I think the best place to set it would be sylius_grid.drivers.orm configuration.

Also, really cool work on https://github.com/vvasiloi/doctrine-pagination! I wonder if there's some solution that is both performant and accurate? Can it be solved somehow with a custom query (for cases like Sylius/Sylius#3775, when using output walkers takes too much time)?

vvasiloi · 2021-08-09T10:46:25Z

@AdamKasp Thanks for picking it up, I forgot about it... 😞 Do you need anything from me?

vvasiloi · 2021-08-12T08:21:12Z

The "use output walker" parameters should allow null, because Doctrine allows it to be null, in which case it will use query hints to determine if an output walker should be used.

…arguments configurable per grid and add default value for all grids

lchrusciel · 2021-08-18T08:21:56Z

src/Bundle/Doctrine/ORM/Driver.php

        if (!isset($configuration['repository']['method'])) {
-            return new DataSource($repository->createQueryBuilder('o'));
+            return new DataSource($repository->createQueryBuilder('o'), $fetchJoinCollection, $useOutputWalkers);


Just a minor thought, but perhaps it would be better to pass the whole configuration array into the data source?

I have no strong opinion about that change. But now, it looks that every operation on the configuration array occurs in Driver, so it makes sense to do it there and pass only proper parameters to DataSource. Do you see any benefits or potential benefits of changing it?

lchrusciel · 2021-08-18T09:52:47Z

Thank you, Victor, Adam and Grzegorz! 🎉

… GridBundle 1.10 (AdamKasp, GSadee) This PR was merged into the 1.9 branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? |master Should be merged after merge PR Sylius/SyliusGridBundle#68 and release new version of `SyliusGridBundle`  Commits ------- e489775 docs about fetch join collection and output walkers d70f712 [Documentation] Improve note about disabling pagination optioons

vvasiloi · 2021-08-18T11:09:37Z

Thank you, Adam and Grzegorz!
I will try to not abandon my PRs like that anymore.

lchrusciel requested review from pamil, GSadee and lchrusciel and removed request for GSadee October 25, 2020 12:53

lchrusciel reviewed Nov 7, 2020

View reviewed changes

src/Bundle/Doctrine/ORM/DataSource.php Outdated Show resolved Hide resolved

pamil force-pushed the pagination branch 4 times, most recently from 05ab13d to cf7f221 Compare December 2, 2020 18:37

pamil approved these changes Dec 2, 2020

View reviewed changes

lchrusciel mentioned this pull request Jul 9, 2021

Shop: Product filtering by taxon broken Sylius/Sylius#12792

Closed

AdamKasp force-pushed the pagination branch 6 times, most recently from f13d73f to 2788d8b Compare August 9, 2021 10:41

AdamKasp mentioned this pull request Aug 9, 2021

[WIP] Pagination #170

Closed

AdamKasp force-pushed the pagination branch 2 times, most recently from 14c1308 to 09aeec0 Compare August 10, 2021 10:32

AdamKasp mentioned this pull request Aug 12, 2021

[Documentation] Note about changes related to upgrade to GridBundle 1.10 Sylius/Sylius#12881

Merged

vvasiloi and others added 5 commits August 17, 2021 12:42

make doctrine orm paginator fetchJoinCollection and useOutputWalkers …

3181988

…arguments configurable per grid and add default value for all grids

rebase with master

7f138be

Make parameters fetchJoinCollection and useOutputWalkers true by default

168a7ac

add fixtures to test output walkers

304f83f

Use PHP 7.4 syntax

460d84f

GSadee force-pushed the pagination branch from 09aeec0 to a8e1edf Compare August 18, 2021 06:55

GSadee added 2 commits August 18, 2021 08:57

Remove BookTranslation from tests

88f7453

Add tests for enabled and disabled use output walkers parameter

b0b0c22

GSadee force-pushed the pagination branch from a8e1edf to b0b0c22 Compare August 18, 2021 06:57

GSadee marked this pull request as ready for review August 18, 2021 06:58

GSadee changed the title ~~[WIP] Fix pagination issue~~ Add possibility to set pagination options on grids Aug 18, 2021

GSadee added the Bug Confirmed bugs or bugfixes. label Aug 18, 2021

lchrusciel approved these changes Aug 18, 2021

View reviewed changes

Fix typed properties of Filter class

a2a3423

lchrusciel approved these changes Aug 18, 2021

View reviewed changes

lchrusciel merged commit 0446613 into Sylius:master Aug 18, 2021

vvasiloi deleted the pagination branch April 16, 2024 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to set pagination options on grids #68

Add possibility to set pagination options on grids #68

vvasiloi commented Oct 18, 2020 •

edited by GSadee

vvasiloi commented Oct 18, 2020

lchrusciel left a comment

pamil left a comment

pamil commented Dec 2, 2020

vvasiloi commented Aug 9, 2021

vvasiloi commented Aug 12, 2021

lchrusciel Aug 18, 2021

GSadee Aug 18, 2021

lchrusciel commented Aug 18, 2021 •

edited

vvasiloi commented Aug 18, 2021

Add possibility to set pagination options on grids #68

Add possibility to set pagination options on grids #68

Conversation

vvasiloi commented Oct 18, 2020 • edited by GSadee

vvasiloi commented Oct 18, 2020

lchrusciel left a comment

Choose a reason for hiding this comment

pamil left a comment

Choose a reason for hiding this comment

pamil commented Dec 2, 2020

vvasiloi commented Aug 9, 2021

vvasiloi commented Aug 12, 2021

lchrusciel Aug 18, 2021

Choose a reason for hiding this comment

GSadee Aug 18, 2021

Choose a reason for hiding this comment

lchrusciel commented Aug 18, 2021 • edited

vvasiloi commented Aug 18, 2021

vvasiloi commented Oct 18, 2020 •

edited by GSadee

lchrusciel commented Aug 18, 2021 •

edited