Skip to content

Enforce sort direction in dql. Do not allowing ordering when disabled. - #389

Merged
curry684 merged 2 commits into
omines:masterfrom
craigh:388-fix-orderby
Jul 23, 2025
Merged

Enforce sort direction in dql. Do not allowing ordering when disabled.#389
curry684 merged 2 commits into
omines:masterfrom
craigh:388-fix-orderby

Conversation

@craigh

@craigh craigh commented Jul 14, 2025

Copy link
Copy Markdown
Contributor

closes #388

@codecov

codecov Bot commented Jul 14, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.52%. Comparing base (1555e12) to head (d39e207).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #389   +/-   ##
=======================================
  Coverage   94.52%   94.52%           
=======================================
  Files          40       40           
  Lines        1078     1078           
=======================================
  Hits         1019     1019           
  Misses         59       59           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Chris53897

Copy link
Copy Markdown
Contributor

@craigh Could you please check the default-order?

The actual code changed our existing sorting. I need to investigate further

@Chris53897

Copy link
Copy Markdown
Contributor

Could you please add a mb_strtolower() for comparison.

We use in our code
->addOrderBy('fieldA', 'ASC')

Comment thread src/DataTable.php
Comment thread src/DataTableState.php
@craigh

craigh commented Jul 17, 2025

Copy link
Copy Markdown
Contributor Author

@Chris53897 done

@craigh

craigh commented Jul 22, 2025

Copy link
Copy Markdown
Contributor Author

ping @curry684

$state->setLength(10);
$state->setGlobalSearch('foo');
$state->setOrderBy([[0, 'asc'], [1, 'desc']]);
$state->setOrderBy([

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.

As noted at #388 (comment) I would revert injecting semi-SQL here as it's confusing. Do the input sanitation tests in the new test case.

@curry684
curry684 merged commit 482c8d8 into omines:master Jul 23, 2025
9 checks passed
@curry684

Copy link
Copy Markdown
Member

I'll merge it and shuffle the tests a bit.

@curry684

Copy link
Copy Markdown
Member

Hrm no I'm reverting it as it's going to cause a lot of new problems. The ordering option only controls whether the user can change the ordering, it's a DataTables option. Setting it to false should NOT block the developer from applying a fixed ordering.

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.

unsafe orderBy usage

3 participants