datagrid
datagrid copied to clipboard
NextrasDataSource faulty checks
Although NextrasDatasource
accepts ICollection
in constructor, comparisons for Nextras\Orm v4 added in #920 and #937 take into account only DbalCollection
.
One would expect ICollection
implementations usable.
The best option is drop support for v3.1, leave v4 only and refactor it to ICollection
everywhere. Current state is not perfect at all, but it's good compromise between backward compatibility with v3.1 and v4.
I am in. Let's support only v4.
My point wasn't about v3/v4 difference, but inpropriate collection type enforcement.
This can happen when you use Nextras\Orm\Repository->findByIds()
. Although DbalCollection
is usually returned, there are cases when ArrayCollection
is returned instead.
https://github.com/contributte/datagrid/blob/1b0ff6a229d471df21ef7a64132f9ebe76a8d947/src/DataSource/NextrasDataSource.php#L130-L138
https://github.com/contributte/datagrid/blob/1b0ff6a229d471df21ef7a64132f9ebe76a8d947/src/DataSource/NextrasDataSource.php#L143-L149
The code above could be changed that the second snippet (DbalCollection ordering) is executed only on DballCollection
instance:
if ($this->dataSource instanceof $this->dbalCollectionClass) {
$order = $this->dataSource->getQueryBuilder()->getClause('order');
if (ArraysHelper::testEmpty($order)) {
$this->dataSource = $this->dataSource->orderBy(
$this->prepareColumn($this->primaryKey)
);
}
}
@mskocik See https://github.com/contributte/datagrid/pull/1062. All good?
@radimvaculik Should be fine according to changes in PR. Haven't tested it yet on real code.
@mskocik Have you tested? Should we close this issue?