Skip to content

Make sort field and direction parameter names available as kernel parameters - #846

Open
mpdude wants to merge 1 commit into
KnpLabs:masterfrom
mpdude:sort-fields-as-kernel-param
Open

Make sort field and direction parameter names available as kernel parameters#846
mpdude wants to merge 1 commit into
KnpLabs:masterfrom
mpdude:sort-fields-as-kernel-param

Conversation

@mpdude

@mpdude mpdude commented Oct 2, 2025

Copy link
Copy Markdown
Contributor

I have repeatedly come across the use case where I need to display a "filter" form above a list that is paginated.

It would be nice if the user could re-sort the list, then afterwards change the filter criteria and submit the filter form again while retaining the sort criteria.

To make that possible, the filter form needs to be aware of the current sort field and direction. In other words, it needs to include hidden form fields for those two query parameters.

It would be much easier to implement this in a generic fashion (with a Symfony FormTypeExentsion) if the parameter names for the sort field and direction were available in the kernel as parameters.

This does not yet solve the special case of having "local" (non-default) parameter names, at least that would require extra configuration for the Symfony FormType. But for "default" lists, the FormTypeExtension could work out-of-the-box.

@garak

garak commented Oct 2, 2025

Copy link
Copy Markdown
Collaborator

A bit of context?

@mpdude

mpdude commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

Hey, you're too fast :) I was still typing 🍻

@garak
garak requested a review from Copilot October 3, 2025 16:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request makes the sort field and direction parameter names available as kernel parameters to enable easier integration with Symfony forms. This allows developers to create generic FormTypeExtensions that can include hidden form fields for sort criteria when building filter forms above paginated lists.

  • Adds kernel parameters for default sort field and direction parameter names
  • Updates the paginator service configuration to use parameter references instead of direct values

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

'sortFieldParameterName' => $config['default_options']['sort_field_name'],
'sortDirectionParameterName' => $config['default_options']['sort_direction_name'],
'sortFieldParameterName' => '%knp_paginator.default.sort_field_name%',
'sortDirectionParameterName' => '%knp_paginator.default.sort_direction_name%',

Copilot AI Oct 3, 2025

Copy link

Choose a reason for hiding this comment

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

Using parameter references in service method calls is incorrect. The parameter values should be resolved before being passed to the method. Use $container->getParameter() to retrieve the parameter values instead of parameter reference strings.

Suggested change
'sortDirectionParameterName' => '%knp_paginator.default.sort_direction_name%',
'sortDirectionParameterName' => $container->getParameter('knp_paginator.default.sort_direction_name'),

Copilot uses AI. Check for mistakes.
@mpdude

mpdude commented Oct 12, 2025

Copy link
Copy Markdown
Contributor Author

Copilot is right about this one :-)

@garak

garak commented Oct 13, 2025

Copy link
Copy Markdown
Collaborator

Copilot is right about this one :-)

So, do you think you're going to apply the suggested change?

@mpdude

mpdude commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

Oh, I missed its comment. I was referring to the summary it wrote above 😀

In fact, I am unsure about the parameter resolution at compile time and think it's right the way I suggested it.

@mpdude

mpdude commented Nov 24, 2025

Copy link
Copy Markdown
Contributor Author

@garak I think my change is fine as suggested.

WDYT?

@garak

garak commented Jan 27, 2026

Copy link
Copy Markdown
Collaborator

Hey @mpdude I'm really sorry that this PR was left behind.
I'm still trying to understand the real value brought to the lib.

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