Cloned articles break previous and next links

NicolasGraph · 2016-02-21 14:02:41

Hi,
In my Txp 4.6 admin, cloned articles do not appears in the articles flow when I click the previous and/or next buttons.
I also have a public side issue with the <txp:link_to_prev /> and <txp:link_to_next /> tags but I’m not sure if it is related.

Last edited by NicolasGraph (2016-02-21 18:02:16)

Bloke · 2016-02-21 17:20:22

NicolasGraph wrote #297949:

duplicated articles do not appears in the articles flow when I click the previous and/or next buttons.
I also have a public side issue with the <txp:link_to_prev /> and <txp:link_to_next /> tags but I’m not sure if it is related.

They’re both the same issue. If you have two articles with absolutely identical timestamps, how can Textpattern know which comes first when comparing timestamps?

If you step forwards, it shows the first article it finds (the “first” article you saved, because it has a lower ID).
If you step backwards it shows the first article it finds (the “second” article you saved because it has a higher ID).

This is the same behaviour in 4.5.x too. As far as I know, there’s no simple fix as it would have to take the IDs into account too.

Workaround: alter the timestamp of one of the articles, even by 1 second.

Note to everyone: what’s the feeling about adding a second by default when cloning? Or (devs) finding a way to take IDs into account when stepping through articles in getNeighbour()?

NicolasGraph · 2016-02-21 18:13:13

Bloke wrote #297950:

They’re both the same issue. If you have two articles with absolutely identical timestamps, how can Textpattern know which comes first when comparing timestamps?

This is the same behaviour in 4.5.x too. As far as I know, there’s no simple fix as it would have to take the IDs into account too.

Thanks Stef, I didn’t notice that before as there wasn’t any Clone button but it’s clear now.

Workaround: alter the timestamp of one of the articles, even by 1 second.

Note to everyone: what’s the feeling about adding a second by default when cloning? Or (devs) finding a way to take IDs into account when stepping through articles in getNeighbour()?

Adding a second would be probably fine; in my case setting the timestamp on cloning would also work…

Bloke · 2016-02-21 19:38:02

NicolasGraph wrote #297951:

setting the timestamp on cloning would also work…

Yes, that’s certainly an option. In fact, that makes the notion of automatically adding a second rather difficult, so perhaps we should forget I mentioned that!

etc · 2016-02-21 20:20:37

Bloke wrote #297950:

Workaround: alter the timestamp of one of the articles, even by 1 second.

This does not fix prev/next links weirdness when sorting by other fields.

Or (devs) finding a way to take IDs into account when stepping through articles in getNeighbour()?

I have proposed a workaround some time ago, but accidentally pushed it in a wrong place, sorry.

Bloke · 2016-02-21 21:38:48

etc wrote #297954:

I have proposed a workaround some time ago

Brilliant, thanks. A few ways we can proceed here:

I can manually apply your commit. You’ll need to revert that part of your Tweak pluggable_ui pull request before it can be merged (though the differences between your request and master are large now).
You revert that portion of the pull request and submit a new pull request just to address the next/prev fix.
You close the pluggable_ui pull request entirely and open two new ones; one for pluggable_ui and one for next/prev.
Merge upstream core into your pull request and then do any of the above.

I don’t mind, whichever you find easiest. If you have a copy of the edits safely backed up, the third option might be the least hassle overall because you can clone our master as it is now and reapply your changes in separate branches, then submit pull requests from each branch.

Your next/prev fix should definitely be in core. And I like the look of the pluggable_ui fix too. I’d like to test it, but can’t merge it at present because it’s too far out of sync now with out master branch (sorry). If you can get the two pull requests to us soon, I’ll make it a priority to test and get them merged in, pronto. Thank you.

etc · 2016-02-26 14:38:52

Thanks, Stef, sorry for the delayed reply. I have closed the old request, but had some unresolved conflicts when updating my branch, and no time to resolve them. If you can manually add it somehow – please don’t think twice! Otherwise, I will submit next/prev request next week. And there should be a better approach to pluggable_ui, so this one can wait.

Bloke · 2016-02-27 00:16:22

etc wrote #298030:

If you can manually add it somehow – please don’t think twice!

I just hacked your next/prev changes into my local dev branch. Effectively, I just merged in this commit with master to produce this:

/**
 * Find an adjacent article relative to a provided threshold level.
 *
 * @param  scalar $threshold      The value to compare against
 * @param  string $s              Optional section restriction
 * @param  string $type           Lesser or greater neighbour? Either '<' (previous) or '>' (next)
 * @param  array  $atts           Attribute of article at threshold
 * @param  string $threshold_type 'cooked': Use $threshold as SQL clause; 'raw': Use $threshold as an escapable scalar
 * @return array|bool An array populated with article data, or 'false' in case of no matches
 */

function getNeighbour($threshold, $s, $type, $atts = array(), $threshold_type = 'raw')
{
    global $prefs;
    static $cache = array();

    $key = md5($threshold.$s.$type.join(n, $atts));

    if (isset($cache[$key])) {
        return $cache[$key];
    }

    extract($atts);
    $expired = ($expired && ($prefs['publish_expired_articles']));
    $customFields = getCustomFields();

    // Building query parts; lifted from publish.php.
    $ids = array_map('intval', do_list($id));
    $id = (!$id) ? '' : " AND ID IN (".join(',', $ids).")";
    switch ($time) {
        case 'any':
            $time = "";
            break;
        case 'future':
            $time = " AND Posted > ".now('posted');
            break;
        default:
            $time = " AND Posted <= ".now('posted');
    }

    if (!$expired) {
        $time .= " AND (".now('expires')." <= Expires OR Expires IS NULL)";
    }

    $custom = '';

    if ($customFields) {
        foreach ($customFields as $cField) {
            if (isset($atts[$cField])) {
                $customPairs[$cField] = $atts[$cField];
            }
        }

        if (!empty($customPairs)) {
            $custom = buildCustomSql($customFields, $customPairs);
        }
    }

    if ($keywords) {
        $keys = doSlash(do_list($keywords));

        foreach ($keys as $key) {
            $keyparts[] = "FIND_IN_SET('".$key."', Keywords)";
        }

        $keywords = " AND (".join(" OR ", $keyparts).")";
    }

    $sortdir = strtolower($sortdir);

    // Invert $type for ascending sortdir.
    $types = array(
        '>' => array('desc' => '>', 'asc' => '<'),
        '<' => array('desc' => '<', 'asc' => '>'),
    );

    $type = ($type == '>') ? $types['>'][$sortdir] : $types['<'][$sortdir];

    // Escape threshold and treat it as a string unless explicitly told otherwise.
    if ($threshold_type != 'cooked') {
        $threshold = "'".doSlash($threshold)."'";
    }

    $safe_name = safe_pfx('textpattern');
    $q = array(
        "SELECT ID AS thisid, Section AS section, Title AS title, url_title, UNIX_TIMESTAMP(Posted) AS posted
            FROM $safe_name WHERE ($sortby $type $threshold
            OR $sortby = $threshold AND ID $type $thisid)",
        ($s != '' && $s != 'default') ? "AND Section = '".doSlash($s)."'" : filterFrontPage(),
        $id,
        $time,
        $custom,
        $keywords,
        "AND Status = 4",
        "ORDER BY $sortby",
        ($type == '<') ? "DESC" : "ASC",
        ",ID ",
        ($type == '<') ? "DESC" : "ASC",
        "LIMIT 1",
    );

    $cache[$key] = getRow(join(n.' ', $q));

    return (is_array($cache[$key])) ? $cache[$key] : false;
}

/**
 * Find next and previous articles relative to a provided threshold level.
 *
 * @param  int    $id        The "pivot" article's id; use zero (0) to indicate $thisarticle
 * @param  scalar $threshold The value to compare against if $id != 0
 * @param  string $s         Optional section restriction if $id != 0
 * @return array An array populated with article data
 */

function getNextPrev($id = 0, $threshold = null, $s = '')
{
    if ($id !== 0) {
        // Pivot is specific article by ID: In lack of further information,
        // revert to default sort order 'Posted desc'.
        $atts = filterAtts(array('sortby' => "Posted", 'sortdir' => "DESC"));
        $atts['thisid'] = $id;
    } else {
        // Pivot is $thisarticle: Use article attributes to find its neighbours.
        assert_article();
        global $thisarticle;
        if (!is_array($thisarticle)) {
            return array();
        }

        $atts = filterAtts();
        $atts['thisid'] = $thisarticle['thisid'];
        $m = preg_split('/\s+/', $atts['sort']);

        // If in doubt, fall back to chronologically descending order.
        if (empty($m[0])            // No explicit sort attribute
            || count($m) > 2        // Complex clause, e.g. 'foo asc, bar desc'
            || !preg_match('/^(?:[0-9a-zA-Z$_\x{0080}-\x{FFFF}]+|`[\x{0001}-\x{FFFF}]+`)$/u', $m[0])  // The clause's first verb is not a MySQL column identifier.
        ) {
            $atts['sortby'] = "Posted";
            $atts['sortdir'] = "DESC";
        } else {
            // Sort is like 'foo asc'.
            $atts['sortby'] = $m[0];
            $atts['sortdir'] = (isset($m[1]) && strtolower($m[1]) == 'desc' ? "DESC" : "ASC");
        }

        // Attributes with special treatment.
        switch ($atts['sortby']) {
            case 'Posted':
                $threshold = "FROM_UNIXTIME(".doSlash($thisarticle['posted']).")";
                $threshold_type = 'cooked';
                break;
            case 'Expires':
                $threshold = "FROM_UNIXTIME(".doSlash($thisarticle['expires']).")";
                $threshold_type = 'cooked';
                break;
            case 'LastMod':
                $threshold = "FROM_UNIXTIME(".doSlash($thisarticle['modified']).")";
                $threshold_type = 'cooked';
                break;
            default:
                // Retrieve current threshold value per sort column from $thisarticle.
                $acm = array_flip(article_column_map());
                $key = $acm[$atts['sortby']];
                $threshold = $thisarticle[$key];
                $threshold_type = 'raw';
                break;
        }

        $s = $thisarticle['section'];
    }

    $out['next'] = getNeighbour($threshold, $s, '>', $atts, $threshold_type);
    $out['prev'] = getNeighbour($threshold, $s, '<', $atts, $threshold_type);

    return $out;
}

While it may have fixed the edge cases where data is sorted by some other non-unique value, it doesn’t seem to fix the stepping through using Next/Prev on the Write panel that NicolasGraph mentioned in the OP. If two articles have identical timestamps (i.e. they are duplicated), then stepping Next shows the first and skips over the second one, and stepping Prev shows the second one and skips over the first.

I would have though that ordering by ID as a secondary parameter would have fixed it. Maybe I missed something from your patch? When you get a chance to look at it, please check I’ve patched it properly. If I’ve screwed up, by all means send in a Pull Request and I’ll give it my attention. Thanks.

etc · 2016-02-27 09:22:07

Sorry, I have tested only the public side. It looks like admin-side next/prev links are governed by checkIfNeighbour function, that needs to be amended too.

Edit: that works for me (in txp_article.php):

function checkIfNeighbour($whichway, $sPosted, $ID)
{
    $sPosted = assert_int($sPosted);
    $ID = assert_int($ID);
    $dir = ($whichway == 'prev') ? '<' : '>';
    $ord = ($whichway == 'prev') ? "DESC" : "ASC";

    return safe_field("ID", 'textpattern',
        "Posted $dir FROM_UNIXTIME($sPosted) OR Posted = FROM_UNIXTIME($sPosted) AND ID $dir $ID ORDER BY Posted $ord, ID $ord LIMIT 1");
}

You must also inject $ID in two checkIfNeighbour($whichway, $sPosted, $ID) calls.

Last edited by etc (2016-02-27 09:43:38)

etc · 2016-03-07 13:18:50

Pull request submitted.

philwareham · 2016-03-07 14:42:09

I’ve merged this commit, thanks – please test.

Textpattern CMS

Textpattern CMS support forum

#1 2016-02-21 14:02:41