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.

Sign up for GitHub

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

Jump to bottom

IBX-2839: Fix pagination with ez_render_content_query() #303

Open
wants to merge 3 commits into
base: 1.3
Choose a base branch
from
Open

IBX-2839: Fix pagination with ez_render_content_query() #303

wants to merge 3 commits into from

Conversation

vidarl
Copy link
Member

@vidarl vidarl commented May 3, 2022 •
edited
Loading

Question Answer
JIRA issue IBX-2839
Type feature
Target Ibexa version v3.3
BC breaks no

The problem is that setting the page_param parameter in a template won't work:

{{ ez_render_content_query({
   'query': {
       'query_type': 'SearchWithLimit',
       'assign_results_to': 'productItems'
   },
   'pagination': {
        'enabled': true,
        'limit': 5,
        'page_param' : 'current_page'
    },
   'template': '@ezdesign/full/SearchWithLimitQueryType.html.twig',
  })
}}

Specifying a GET parameter named current_page won't affect the query result :
http://localhost:8080/tests/cs-9997-querytype-and-limit?current_page=2

This is because the twig function will issue a subrequest and the GET variable specified by the page_param parameter won't be passed on to the subrequest :

https://github.com/ezsystems/ezplatform-kernel/blob/v1.3.17/eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/QueryRenderingExtension.php#L37-L39

In this PR I just pass on the page_param to the subrequest

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@vidarl
Copy link
Member Author

vidarl commented May 3, 2022

Still WIP as I haven't added any tests yet.
But @adamwojs : Is this the direction you would prefer, or should I use some other approach ?

@sonarcloud
Copy link

sonarcloud bot commented May 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

vidarl
vidarl commented May 3, 2022
View reviewed changes
eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/QueryRenderingExtension.php Outdated
return new ControllerReference('ez_query_render::renderQuery', [
'options' => $options,
]);
$pageParam = isset($options['pagination']['page_param']) ? $options['pagination']['page_param'] : 'page';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI : The default value for page_param is also hardcoded here : https://github.com/ezsystems/ezplatform-kernel/blob/v1.3.17/eZ/Publish/Core/MVC/Symfony/Controller/QueryRenderController.php#L76-L81

@adamwojs
Copy link
Member

adamwojs commented May 4, 2022

+1 but please adapt failing unit tests

@adamwojs adamwojs marked this pull request as draft May 4, 2022 07:20
@adamwojs adamwojs changed the title WIP : IBX-2839: Fix pagination with ez_render_content_query() IBX-2839: Fix pagination with ez_render_content_query() May 4, 2022
@vidarl vidarl marked this pull request as ready for review July 1, 2022 13:18
@sonarcloud
Copy link

sonarcloud bot commented Jul 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vidarl vidarl requested review from adamwojs and a team July 4, 2022 10:58
@vidarl
Copy link
Member Author

vidarl commented Jul 4, 2022

Existing tests fixed and added one more

@alongosz alongosz requested review from mikadamczyk, Steveb-p and a team July 6, 2022 12:39
@alongosz
Copy link
Member

Hey @vidarl this looks like almost ready. Any progress on applying the requested change. Or is this outdated/not relevant anymore?

@sonarcloud
Copy link

sonarcloud bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vidarl
Copy link
Member Author

vidarl commented Aug 14, 2023

Hey @vidarl this looks like almost ready. Any progress on applying the requested change. Or is this outdated/not relevant anymore?

It is still relevant. Suggestions are now implemented

@vidarl vidarl requested a review from alongosz August 14, 2023 13:40
@vidarl vidarl force-pushed the ibx-2839_ez_render_content_query_and_page_param_parameter branch from 5809cfd to ef2ae98 Compare November 10, 2023 13:22
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vidarl
Copy link
Member Author

vidarl commented Nov 10, 2023

@alongosz : I have now also rebased the PR to latest 2.3. Anything else that needs to be done in order to get approval ?

webhdx
webhdx approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@webhdx webhdx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look okay but when relying on RequestStack and current request I always have concerns how it's going to work inside render_esi fragments and Varnish. Have you checked this case?

...g/Extension/_fixtures/query_rendering_functions/ez_render_content_query_with_pagination.test
@@ -0,0 +1,48 @@
--TEST--
"ez_render_content_query_b" function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean ez_render_content_query?

@webhdx webhdx requested a review from a team June 13, 2024 07:29
@Steveb-p Steveb-p requested a review from a team June 13, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kisztof kisztof kisztof approved these changes

@Steveb-p Steveb-p Steveb-p approved these changes

@webhdx webhdx webhdx approved these changes

@konradoboza konradoboza konradoboza approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

@mikadamczyk mikadamczyk Awaiting requested review from mikadamczyk

@alongosz alongosz Awaiting requested review from alongosz

Assignees

@adamwojs adamwojs

@kisztof kisztof

@mikadamczyk mikadamczyk

@Steveb-p Steveb-p

@alongosz alongosz

@ciastektk ciastektk

Labels
Ready for review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vidarl @adamwojs @alongosz @kisztof @Steveb-p @webhdx @konradoboza @mikadamczyk @ciastektk