When node comments are in threaded view from older to newer, when pruned/grafted to turn a comment into a new node the whole tree below it gets reversed (newer to older instead of the original older to newer). New comments are added in the right order afterwards.

Comments

PMunn’s picture

I just noticed this as well.

PMunn’s picture

Status:Active» Needs review
StatusFileSize
new4.2 KB

Here's a patch that fixes it.

It was using the same awful thread ID generation code I replaced in the phpbb2drupal module in this issue. I've fixed that.

It was also grabbing the comments in reverse thread ID order. This meant it was processing them newest to oldest, but adding them into the target node newest to oldest. Oopsie.

I tested it with a forum post with one level of comments and one with multiple levels of comments to be sure the graft still worked as desired. It did.

Heine’s picture

Status:Needs review» Needs work

Thanks a lot.

The patch has a few problems, however. I've tried it on a new node with threaded comments, oldest first. I've created for comments and gave all comments three children. Next, I made sure that all children of two, three and four and three and four themselves became children of two in one long branch. When finally moving the entire branch two to become a child of comment one, threading was completely off.

Here's an overview of the comment table.

mysql> SELECT cid, pid, thread, subject from comments;

+-----+-----+--------+---------+
| cid | pid | thread | subject |
+-----+-----+--------+---------+
|  35 |   0 | 01/    | 1       |
|  36 |   0 | 02/    | 2       |
|  37 |   0 | 03/    | 3       |
|  38 |   0 | 04/    | 4       |
|  39 |  35 | 01.00/ | 11      |
|  40 |  35 | 01.01/ | 12      |
|  41 |  35 | 01.02/ | 13      |
|  42 |  36 | 02.00/ | 21      |
|  43 |  36 | 02.01/ | 22      |
|  44 |  36 | 02.02/ | 23      |
|  45 |  37 | 03.00/ | 31      |
|  46 |  37 | 03.01/ | 32      |
|  47 |  37 | 03.02/ | 33      |
|  48 |  38 | 04.00/ | 41      |
|  49 |  38 | 04.01/ | 42      |
|  50 |  38 | 04.02/ | 43      |
+-----+-----+--------+---------+
16 rows in set (0.00 sec)

Moving all 2+ comments as a straight branch (child->child->child--//-->child under 2) and all children of 1 as a straight branch under 1).

mysql> SELECT cid, pid, thread, subject from comments;
+-----+-----+--------------------------------------+---------+
| cid | pid | thread                               | subject |
+-----+-----+--------------------------------------+---------+
|  35 |   0 | 01/                                  | 1       |
|  36 |   0 | 02/                                  | 2       |
|  37 |  44 | 02.00.02.01.01/                      | 3       |
|  38 |  47 | 02.00.02.01.01.01.01.01.01/          | 4       |
|  39 |  35 | 01.00/                               | 11      |
|  40 |  39 | 01.00.01/                            | 12      |
|  41 |  40 | 01.00.01.01/                         | 13      |
|  42 |  36 | 02.00/                               | 21      |
|  43 |  42 | 02.00.02/                            | 22      |
|  44 |  43 | 02.00.02.01/                         | 23      |
|  45 |  37 | 02.00.02.01.01.01/                   | 31      |
|  46 |  45 | 02.00.02.01.01.01.01/                | 32      |
|  47 |  46 | 02.00.02.01.01.01.01.01/             | 33      |
|  48 |  38 | 02.00.02.01.01.01.01.01.01.01/       | 41      |
|  49 |  48 | 02.00.02.01.01.01.01.01.01.01.01/    | 42      |
|  50 |  49 | 02.00.02.01.01.01.01.01.01.01.01.01/ | 43      |
+-----+-----+--------------------------------------+---------+

Pruning 2 (36) and grafting on 13 (41)

mysql> SELECT cid, pid, thread, subject from comments;        
+-----+-----+--------------------------------------+---------+
| cid | pid | thread                               | subject |
+-----+-----+--------------------------------------+---------+
|  35 |   0 | 01/                                  | 1       |
|  36 |  41 | 01.00.01.01.01/                      | 2       |
|  37 |  44 | 02.00.02.01.02/                      | 3       |
|  38 |  47 | 02.00.02.01.01.01.01.01.02/          | 4       |
|  39 |  35 | 01.00/                               | 11      |
|  40 |  39 | 01.00.01/                            | 12      |
|  41 |  40 | 01.00.01.01/                         | 13      |
|  42 |  36 | 01.00.01.01.01.01/                   | 21      |
|  43 |  42 | 02.00.03/                            | 22      |
|  44 |  43 | 02.00.02.02/                         | 23      |
|  45 |  37 | 02.00.02.01.01.02/                   | 31      |
|  46 |  45 | 02.00.02.01.01.01.02/                | 32      |
|  47 |  46 | 02.00.02.01.01.01.01.02/             | 33      |
|  48 |  38 | 02.00.02.01.01.01.01.01.01.02/       | 41      |
|  49 |  48 | 02.00.02.01.01.01.01.01.01.01.02/    | 42      |
|  50 |  49 | 02.00.02.01.01.01.01.01.01.01.01.02/ | 43      |
+-----+-----+--------------------------------------+---------+
PMunn’s picture

I'm having a hard time understanding what went on in your example, but I'll try to replicate it on a testbed myself within the next couple of days and see what happens. I might make more snapshots in between that will help me see the problem and why not, others too. Thanks for bringing this to my attention.

PMunn’s picture

StatusFileSize
new4.54 KB

I'm not seeing a problem. Here's what I did:

Created four threads, and then two comments per thread.

thread subject
01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
02/ This should be 02.
02.00/ This should be 02.00.
02.01/ This should be 02.01.
03/ 03.
03.00/ 03.00.
03.01/ 03.01.
04/ 04.
04.00/ 04.00.
04.01/ 04.01.

First, I pruned 03 and grafted it to 02.

thread subject
01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
02/ This should be 02.
02.00/ This should be 02.00.
02.01/ This should be 02.01.
02.02/ 03.
02.02.00/ 03.00.
02.02.01/ 03.01.
04/ 04.
04.00/ 04.00.
04.01/ 04.01.

Then I pruned 04 and grafted it onto 02.

thread subject
01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
02/ This should be 02.
02.00/ This should be 02.00.
02.01/ This should be 02.01.
02.02/ 03.
02.02.00/ 03.00.
02.02.01/ 03.01.
02.03/ 04.
02.03.00/ 04.00.
02.03.01/ 04.01.

And lastly I grafted 02 onto 01:

thread subject
01/ This should be 01.
01.00/ This should be 01.00.
01.01/ This should be 01.01.
01.02/ This should be 02.
01.02.00/ This should be 02.00.
01.02.01/ This should be 02.01.
01.02.02/ 03.
01.02.02.00/ 03.00.
01.02.02.01/ 03.01.
01.02.03/ 04.
01.02.03.00/ 04.00.
01.02.03.01/ 04.01.

The sorts continue to work.

I have updated the patch with a minor fix. The comment vancodes were starting at 01 when they should start at 00. That's it.

Can you explain in finer detail how to reproduce what you've seen.

Heine’s picture

Status:Needs work» Needs review
StatusFileSize
new4.85 KB

An updated patch makes sure the '/' is disregarded when ASC sorting.

PMunn’s picture

StatusFileSize
new4.6 KB

I've snipped the first few lines of the patch which made applying it break. I've seen this before. Some method some developers are using to post patches is tacking on a few lines of information at the top of the file, at least that's how it looks when I download it.

Typically I see patches written with a .patch on the end so I tend to use that suffix on the resulting patch file.

Anyway I tested this patch and the sort is working properly with the SQL change on PostgreSQL.

Heine’s picture

Thank you.

Patch in #6 is created using CVS, so we know exactly what revision it is against. Your patch program should simply ignore this (weird it doesn't).

I'll test a few more scenarios, then commit.

Heine’s picture

Status:Needs review» Patch (to be ported)

Committed to 4.7.x-1.x-dev and HEAD. Will backport to 4.6.

Thanks.

Leeteq’s picture

Status:Patch (to be ported)» Fixed

if 4.6 is not relevant anymore, maybe this issue is fixed?

Anonymous’s picture

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.