Open LiamKearn opened 2 years ago
Any BC concerns are easily resolved by releasing this in a major version update if we're that worried about it. IMO the PHPdoc aligns with my expectation that this would only apply "distinct" to the column we're explicitly interested in and not some other columns that just happen to be in the underlying SQL query.
I personally think it would be prudent to preserve the sort but somehow still make the method unique on a single column. Might involve some SQL technicary thats beyond me.
In CMS 5 sort()
will accept a null
argument to remove sort so that will be a slightly nicer looking workaround, but the main problem still remains that you can't sort before calling columnUnique
and expect a nicely sorted array of actually unique column values, where that uniqueness is related only to the column of interest.
@GuySartorelli just got the issue and your fix works nicely
public function columnUnique($colName = "ID")
{
return $this->dataQuery->distinct(true)->sort(null)->column($colName);
}
currently, it's really silly to have to do array_unique($query->columnUnique(...)) to have something really unique. and it's really not obvious because it seems to work at first glance, but in fact it really doesn't
https://github.com/silverstripe/silverstripe-framework/blob/10ef46a5ec832133892867bedb0cf3097818dd58/src/ORM/DataList.php#L1034-L1043
When I call the following, given that
MyDataObject
has adefault_sort
ofCreated
this will return mostly unique rows:It seems you can work around this if needed via:
so on this topic it might be nice to have a removeSort method etc or allow passing false or null etc to remove default sort instead.
This is especially notable as bad ergonomics for new SS devs. If you have a
default_sort
config on your DataObject this almost happens transparently.Might be a bit of a tricky one in terms of BC and API changes were it to change. I'm personally of the opinion that this should just be docblocked clearly now to avoid breaking changes (not because I like the current API). @GuySartorelli classed this as a bug and might have some other thoughts.