Skip to content

Fix grid sorting bc break#372

Merged
NoResponseMate merged 1 commit into
Sylius:1.14from
Florian-Merle:fix-grid-sorting-bc-break
May 26, 2025
Merged

Fix grid sorting bc break#372
NoResponseMate merged 1 commit into
Sylius:1.14from
Florian-Merle:fix-grid-sorting-bc-break

Conversation

@Florian-Merle
Copy link
Copy Markdown
Contributor

This fixes #369

Comment on lines 26 to 34
foreach ($gridConfiguration['sorting'] as $sorting => $order) {
Assert::keyExists($gridConfiguration['fields'] ?? [], $sorting);

if (isset($gridConfiguration['fields'][$sorting]['sortable'])) {
continue;
}

$gridConfiguration['fields'][$sorting]['sortable'] = true;
}
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.

I'm wondering if using isset here is the right solution.

Looking at the tests, I had to update the line for the author because its sortable value was originally being changed from false to true. However, with these changes, that no longer happens.

Maybe the value should be set to true if it's either not set or currently false.

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.

maybe should we throw an error that indicates the field is not sortable.

@Florian-Merle Florian-Merle force-pushed the fix-grid-sorting-bc-break branch from 350f572 to 635f9b9 Compare April 21, 2025 15:03
@loic425
Copy link
Copy Markdown
Member

loic425 commented May 26, 2025

@Florian-Merle Is this one ok for you?

@loic425
Copy link
Copy Markdown
Member

loic425 commented May 26, 2025

@Florian-Merle I've tried the change on that grid and that fixes the issue.
Sylius/SyliusResourceBundle#1019

@NoResponseMate NoResponseMate merged commit 6f17639 into Sylius:1.14 May 26, 2025
10 checks passed
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.

BC break related to sorting in v1.14.0-ALPHA.1

4 participants