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

[Warning] Array to string conversion #344

Closed
axllent opened this issue Jul 1, 2022 · 3 comments
Closed

[Warning] Array to string conversion #344

axllent opened this issue Jul 1, 2022 · 3 comments

Comments

@axllent
Copy link

axllent commented Jul 1, 2022

PHP produces a warning in relation to PR #325 when default_sort is an array, eg:

private static $default_sort = [
    '"SortOrder"' => 'ASC',
];

$sortterm .= ($sortterm ? ', ' : '') . $defaultSort; incorrectly assumes $default_sort is always a string ~ it can be either a string or an array.

CC @davejtoews

@axllent
Copy link
Author

axllent commented Jul 6, 2022

It pains me to come back to this with my head low, however this is not actually a bug at all, more like an undocumented (and only partially working) Silverstripe feature. Although using arrays for $default_sort do actually work within Silverstripe, the API documentation and DataObject:class source code clearly specify the value should be a string, and nowhere can I (now) find reference to it ever being an array.

The best I can say is, $default_sort is not supposed to be an array, even if it does work. I shall now close this issue, and apologies for my mistake.

@axllent axllent closed this as completed Jul 6, 2022
obj63mc pushed a commit to obj63mc/silverstripe-menu that referenced this issue Aug 16, 2022
Currently if on php 8.1 and the latest version of SS - Line 370 in /app/vendor/symbiote/silverstripe-gridfieldextensions/src/GridFieldOrderableRows.php throws an error as the sort is expected to be a string.  Setting to a default convention which will work.  See - symbiote/silverstripe-gridfieldextensions#344 for more details
Dennisprins93 pushed a commit to wedevelopnl/silverstripe-articles that referenced this issue Aug 24, 2022
@NightJar
Copy link
Contributor

I think this is valid & should be reopened. Although the DocBlock on DataObject might state that the parameter should be a string, it is a case of classic poorly updated documentation and obtuse APIs.

For instance, the DocBlock on the private static variable states it should be a string, then immediately instantiates it to null.

DataQuery::sort is the main place this is used, but the DocBlock there states it expects fully escaped raw SQL as the parameter (likely from purely historic reasons).
It also expects direction as the second parameter, which isn't set (but could be a part of the so-called SQL statement).

It then delegates though SQLSelect::setOrderBy to SQLSelect::addOrderBy - both of which state explicitly they accept quite an accepting set of parameters and has an allowance for the direction.

     * @example $sql->addOrderBy("Column");
     * @example $sql->addOrderBy("Column DESC");
     * @example $sql->addOrderBy("Column DESC, ColumnTwo ASC");
     * @example $sql->addOrderBy("Column", "DESC");
     * @example $sql->addOrderBy(["Column" => "ASC", "ColumnTwo" => "DESC"]);

Then contradicting years of this type of use, DataObjectSchema goes on to only accept strings... but only since 4.1.0!

Classic Silverstripe contradictory support due to poorly or undefined APIs - perhaps due to overusing config, which is a large free-form key-value database (a bit like using excel instead of a database). I argue the solution is to make the API better defined, and more in line with other common and already supported use cases.

While this probably isn't the correct repository for the above argument, but I think it is highly expected this module should make allowance for arrays as the default_sort DataObject configuration value. I.e. the complaint is valid, even if the Framework hints otherwise (I claim it is historic and now irrelevant).

@GuySartorelli
Copy link
Collaborator

fixed in 3.5.1 and 3.6.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants