silverstripe / silverstripe-asset-admin

Silverstripe assets gallery for asset management
BSD 3-Clause "New" or "Revised" License
20 stars 79 forks source link

Unable to sort by Title when using Fluent Versioned extension #1426

Closed aria5305 closed 9 months ago

aria5305 commented 9 months ago

Description:

Sorting by Title (ASC or DESC) will resolve in a error message and files are unable to sort when TractorCow\Fluent\Extension\FluentVersionedExtension is added to SilverStripe\Assets\File

image

You can still sort by Oldest and Newest

Removing the extension and sorting will work normally.

Expected behaviour: Able to sort by Title without receiving errors

Packages: "name": "silverstripe/asset-admin", "version": "1.13.9", "name": "tractorcow/silverstripe-fluent", "version": "6.0.0", "name": "silverstripe/graphql","version": "4.3.5",

PRs

GuySartorelli commented 9 months ago

Please raise this issue in the tractorcow/silverstripe-fluent repository.

aria5305 commented 9 months ago

@GuySartorelli Hmmm, I did think about whether this belongs in Fluent module 🤔

Just a quick question here: For sortChildren, should the sort() be taking in $normalised instead of $field so that's sorting with the field that matches the casing declared in the Object? or is there some reason we should by sorting with $field?

    foreach ($sortArgs as $field => $dir) {
                    $normalised = FieldAccessor::singleton()->normaliseField(File::singleton(), $field);
                    Schema::invariant(
                        $normalised,
                        'Could not find field %s on %s',
                        $field,
                        File::class
                    );
                    $dataQuery->sort($field, $dir, false);
                }
GuySartorelli commented 9 months ago

Hmmm, I did think about whether this belongs in Fluent module 🤔

As a general rule of thumb, if the functionality works fine until a certain piece of functionality in a specific module is applied, the bug should be logged against that module. The only case that isn't true is if you can describe and replicate the bug in more generic ways that don't rely on modules than the one you want to raise the issue against.

For sortChildren, should the sort() be taking in $normalised instead of $field so that's sorting with the field that matches the casing declared in the Object? or is there some reason we should by sorting with $field?

Are you talking specifically about FolderTypeResolver::sortChildren()?

If so, the answer is: I have no context to what normaliseField() is, so I can't say for sure. It does looks likely that the intention was to pass the $normalised variable into the sort() method of the query, though.

aria5305 commented 9 months ago

@GuySartorelli Ah okay, that's good to know. I will remember that, thank you :)

Sorry was rushing a bit before so didn't give full context, but yes I was referring to FolderTypeResolver::sortChildren().

We overrode the default sortChildren method and instead of using $field, we passed on $normalised to sort() and that seem to have resolved the issue. Would this be a fix for the asset-admin module then?

GuySartorelli commented 9 months ago

Would this be a fix for the asset-admin module then?

Sounds like it, if it resolved the problem. Feel free to raise a pull request and I'll see if I can both reproduce the original bug and see the PR fix that bug.

aria5305 commented 9 months ago

Awesome, I will do this later today when I have some time, thanks @GuySartorelli

GuySartorelli commented 9 months ago

PR merged. It'll be automatically tagged by GitHub actions.