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.

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

[Tree] Allow multiple order critieria in reorder and reorderAll #2744

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ a release.
### Deprecated
- Sluggable: Annotation-specific mapping parameters (#2837)

### Changed
- Allow `string[]` as `$sortByField` and `$direction` parameter in `\Gedmo\Tree\Entity\Repository\NestedTreeRepository::reorder()` and `\Gedmo\Tree\Entity\Repository\NestedTreeRepository::reorderAll()`

Comment on lines +24 to +26
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
### Changed
- Allow `string[]` as `$sortByField` and `$direction` parameter in `\Gedmo\Tree\Entity\Repository\NestedTreeRepository::reorder()` and `\Gedmo\Tree\Entity\Repository\NestedTreeRepository::reorderAll()`
### Changed
- Allow `string[]` as `$sortByField` and `$direction` parameter in `\Gedmo\Tree\Entity\Repository\NestedTreeRepository::reorder()` and `\Gedmo\Tree\Entity\Repository\NestedTreeRepository::reorderAll()`


### Fixed
- Fix regression with `doctrine/dbal` >= 4.0 that caused MariaDB to improperly attempt LONGTEXT casting in `TranslationWalker` (issue #2887)
- Tree: allow usage of UuidV4 as path source with the materialized path strategy
Expand Down
16 changes: 8 additions & 8 deletions src/Tree/Entity/Repository/NestedTreeRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -801,11 +801,11 @@ public function removeFromTree($node)
* Reorders $node's child nodes,
* according to the $sortByField and $direction specified
*
* @param object|null $node node from which to start reordering the tree; null will reorder everything
* @param string $sortByField field name to sort by
* @param string $direction sort direction : "ASC" or "DESC"
* @param bool $verify true to verify tree first
* @param bool $recursive true to also reorder further descendants, not just the direct children
* @param object|null $node node from which to start reordering the tree; null will reorder everything
* @param string|string[]|null $sortByField Field name or array of fields names to sort by
* @param string|string[] $direction Sort order ('asc'|'desc'|'ASC'|'DESC'). If $sortByField is an array, this may also be an array with matching number of elements
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change allowing the combination of string and string[] between these parameters? I mean by instance $sortByField = ['a', 'b'], $direction = 'ASC'.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it does. The reorder functions does nothing with the $sortByField and $direction parameter directly but passes them to NestedTreeRepository::children(...) where those parameters a passsed again (and again) till the function NestedTreeRepository::childrenQueryBuilder(...) is reached. Where in line 283++ those cases are handled

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the context.
Could you please add a test case for this scenario?

* @param bool $verify true to verify tree first
* @param bool $recursive true to also reorder further descendants, not just the direct children
*
* @return void
*/
Expand Down Expand Up @@ -836,9 +836,9 @@ public function reorder($node, $sortByField = null, $direction = 'ASC', $verify
/**
* Reorders all nodes in the tree according to the $sortByField and $direction specified.
*
* @param string $sortByField field name to sort by
* @param string $direction sort direction : "ASC" or "DESC"
* @param bool $verify true to verify tree first
* @param string|string[]|null $sortByField Field name or array of fields names to sort by
* @param string|string[] $direction Sort order ('asc'|'desc'|'ASC'|'DESC'). If $sortByField is an array, this may also be an array with matching number of elements
* @param bool $verify true to verify tree first
*
* @return void
*/
Expand Down
25 changes: 25 additions & 0 deletions tests/Gedmo/Tree/NestedTreeRootRepositoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,31 @@ public function testShouldHandleAdvancedRepositoryFunctions(): void
static::assertSame(9, $node->getLeft());
static::assertSame(10, $node->getRight());

// reorder (multiple order critieria)

$node = $repo->findOneBy(['title' => 'Vegitables']);
$repo->reorder($node, ['level', 'title'], ['ASC', 'DESC'], false, false);

$node = $repo->findOneBy(['title' => 'Cabbages']);

static::assertSame(9, $node->getLeft());
static::assertSame(10, $node->getRight());

$node = $repo->findOneBy(['title' => 'Carrots']);

static::assertSame(7, $node->getLeft());
static::assertSame(8, $node->getRight());

$node = $repo->findOneBy(['title' => 'Onions']);

static::assertSame(5, $node->getLeft());
static::assertSame(6, $node->getRight());

$node = $repo->findOneBy(['title' => 'Potatoes']);

static::assertSame(3, $node->getLeft());
static::assertSame(4, $node->getRight());

// reorder

$node = $repo->findOneBy(['title' => 'Food']);
Expand Down
Loading