bolt / bolt

Bolt is a simple CMS written in PHP. It is based on Silex and Symfony components, uses Twig and either SQLite, MySQL or PostgreSQL.
MIT License
4.15k stars 809 forks source link

.previous() and .next() return unexpected results when sorting by a field with non-unique values #6129

Closed jerryjappinen closed 6 years ago

jerryjappinen commented 7 years ago

In contenttypes.yml I have defined the following content type, where each record has a uniqueish title and a year value:

# Archived side projects
archives:
    listing_sort: "-year"
    sort: "-year"
    fields:
        title:
            type: text
        year:
            type: integer

Note that I'm not using a timestamp, just a year number, and many records of this type will have the same value. I wish to sort my records by year in my list view, and provide consistent next/prev navigation on the record page.

However, .next() will not get the next record in the list sorted by year. It will instead get the first record with a higher year value than the current record. Same thing with previous in the other direction of course. These are often the same thing when sorting by timestamps or unique titles, but not with recurring values like a year.

As an example, if I have the following list of records (the titles don't matter):

index   year    title

[0]     2017    Dying Light
[1]     2016    The Darkness II
[2]     2016    Tomb Raider
[3]     2015    Dragon Age Inquisition
[4]     2015    Crimsonland
[5]     2015    Geometry Wars 3
[6]     2015    Grim Fandango
[7]     2015    Fallout New Vegas
[8]     2014    Metal Gear Solid 2
[9]     2014    Call of Duty Advanced Warfare

Sorting works fine. But .previous() and .next() will look for the first record where the year value is higher/lower:

{% setcontent archives = 'archives' %}
{% set crimsonland = archives[4] %}

{# Expecting 'Dragon Age Inquisition', but gives me 'The Darkness II' #}
{% set prev = crimsonland.previous() %}

{# Expecting 'Geometry Wars 3', but gives me 'Metal Gear Solid 2' #}
{% set next = crimsonland.next() %}

Things will completely break down if I sort with -year,title: both functions will give me 'Call of Duty Advanced Warfare'.

As it is the prev/next functions work for many common use cases where the field values are mostly unique, but not with recurring values like I have here.

Other notes

The sorting behavior is consistent regardless of where I do the sorting: I get the same results if I use -year in setcontent and pass it as a parameter to record.previous()/record.next(). I took a look into src/Legacy/Content.php, and it seems this is indeed the intended behavior:

public function previous($field = 'datepublish', $where = [])
{
    list($field, $asc) = $this->app['storage']->getSortOrder($field);

    $operator = $asc ? '<' : '>';
    $order = $asc ? ' DESC' : ' ASC';

    $params = [
        $field         => $operator . $this->values[$field],
        'limit'        => 1,
        'order'        => $field . $order,
        'returnsingle' => true,
        'hydrate'      => true,
    ];

    $pager = [];
    $previous = $this->app['storage']->getContent($this->contenttype['singular_slug'], $params, $pager, $where);

    return $previous;
}

While I'm not 100 % familiar with how the parameters are used, it looks to me like this code will look for the first element that has a higher/lower value in the specified field than the original record (in my case year). The implementation isn't honestly looking for the next element relative to the current one in the list that the user is dealing with, in effect skipping over many records.

I marked this as a bug since when encountering this issue and getting support, this seemed to be a surprising and not the expected behavior.

bobdenotter commented 7 years ago

Confirmed.. What you describe is correct, but i'm not sure how we're going to fix this in an efficient manner.

I've worked on a CMS before that had a proper implementation of this, by literally going over the entire result-set, and when the current item was encountered, it returned the previous or next one. It worked, but as you can imagine the performance was terrible if you have a few thousand items.

jerryjappinen commented 7 years ago

Ok, hope we can get this resolved! Now I have no idea how the current implementation works and it's been a while since I've written SQL, but here's a couple of ideas/questions:

Kinda thinking out loud, not sure if any of this makes sense.

bobdenotter commented 7 years ago

I think with most of your suggestions you'd bump into the limitation i've mentioned: If for every next you'd have to iterate over all items, it'd become a performance hog.

Another suggestion: If sorting on a "broad" field, like only a year, we could perhaps add the id as an extra sort. Sorting might still be a tiny bit peculiar, but at least it'll give the same results every time.

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. Maybe this issue has been fixed in a recent release, or perhaps it is not affecting a lot of people? It will be closed if no further activity occurs, because we like to keep the issue queue concise and actual. If you think this issue is still relevant, please let us know. Especially if you’d like to help resolve the issue, either by helping us pinpointing the cause of a bug, or in implementing a fix or new feature.