[Bug] Nested comments - undefined index in Tree about foscommentbundle HOT 18 CLOSED

Can you give me more information about your configuration? I tested the sorters when writing them and the tree should be built prior to any sorting operation.

from foscommentbundle.

Here you go: https://gist.github.com/1117849

Sorter is indeed run after building of the tree, but I am talking about the SQL ORDER BY which is done prior:

        $comments = $this->findCommentsByThread($thread, $depth); // leaves parent node last
        $sorter = $this->getSortingFactory()->getSorter($sorter);

        return $this->organiseComments($comments, $sorter); // fails on first node, since parent is not in tree

I'm unaware how different DBMS' handle ORDER BY where NULLs are concerned, Postgre on my debian box leaves them last ..

from foscommentbundle.

The sorter isnt passed into findCommentsByThread(), it only gets run when passing it through the organiseComments function.

Are you seeing a specific error?

from foscommentbundle.

I was not particularly clear I think. The sorter has got nothing to do with the error, it is the ORDER BY which is run inside findCommentsByThread I issue a var_dump right before the foreach in organiseComments(), and here is what I get (a bit stripped down):

array
  0 => 
    object(App\CommentBundle\Entity\Comment)[1911]
      protected 'id' => int 4
      protected 'thread' => 
        object(FOS\CommentBundle\Entity\Thread)[1337]
          protected 'identifier' => string 'foo' (length=3)
          protected 'isCommentable' => boolean true
          protected 'numComments' => int 2
          protected 'lastCommentAt' => 
            object(DateTime)[1332]
              ...
      protected 'ancestors' => string '3' (length=1)
      protected 'parent' => null
      protected 'body' => string 'child' (length=5)
      protected 'depth' => int 1
  1 => 
    object(App\CommentBundle\Entity\Comment)[1903]
      protected 'id' => int 3
      protected 'thread' => 
        object(FOS\CommentBundle\Entity\Thread)[1337]
          protected 'identifier' => string 'foo' (length=3)
          protected 'isCommentable' => boolean true
          protected 'numComments' => int 2
          protected 'lastCommentAt' => 
            object(DateTime)[1332]
              ...
      protected 'ancestors' => null
      protected 'parent' => null
      protected 'body' => string 'parent' (length=6)
      protected 'depth' => int 0

If you check the organiseComments() code, in case ancestors exist (they do, for $comments[0]), $tree->traverse($ancestorId) is issued, which leads to error, since such ancestor has not yet been added to the tree.

Concete error message:
An exception has been thrown during the rendering of a template ("Notice: Undefined index: 3 in /home/dev/vendor/bundles/FOS/CommentBundle/Model/Tree.php line 60") in "FOSCommentBundle:Thread:show.html.twig" at line 25.

from foscommentbundle.

Ooh. After writing out a reply about how it is supposed to work i see what you're talking about.

merk@4e33940

Can you try applying that commit (branch sorting_fix on my repository) to see if it fixes your issue?

from foscommentbundle.

Confirm, fixed. Although, a perhaps simpler solution would be to sort by c.Id in the first place?

from foscommentbundle.

However unlikely, (and not really possible through the interfaces provided), the data structure can have a parent comment that is older.

It feels safer to sort by the ancestors.

from foscommentbundle.

While true, also ancestor can refer to a comment that is newer in that case ;)

from foscommentbundle.

Sorting by ancestor will always mean that the parent has been added though, children are always going to come after the parent.

I've committed the fix to master, thanks for reporting. :)

from foscommentbundle.

Cheers & thanks :)

from foscommentbundle.

Sorry, I must have messed something up, your fix actually does not work, this is not valid SQL: ORDER BY c.ancestors IS NULL DESC, c.ancestors ASC

Best option would be to use c.ancestors ASC NULLS FIRST, but QueryBuilder does not support it, any ideas? I'd actually still use ->orderBy('c.id') or perhaps even ->orderBy('c.createdat')? Safest option is to do it on PHP side, that doesn't seem too efficient tho ..

from foscommentbundle.

I suspect ordering by a creation date is going to be as good as it gets in the short term. I'll look into what can be done tomorrow.

What database engine are you using? ORDER BY (c.ancestors IS NULL) DESC works in MySQL.

from foscommentbundle.

Postgre, but the one complaining is Doctrine

from foscommentbundle.

@merk Don't forget you are not writing SQL but DQL here.

from foscommentbundle.

@merk Don't forget you are not writing SQL but DQL here.

from foscommentbundle.

I digged some further into ORDER BY NULLS and figured it is not the best solution for a public bundle - supported afaik only by Postgre&Oracle. Albeit all that, I've created a PR to doctrine, which probably gets shot down, but perhaps it raises a thread how to solve such a need.

from foscommentbundle.

How about changing ancestors field to not-nullable (empty string for parents) (thanks @lenar)? Fixes the ordering issue and might be semantically more correct, since NULL refers to value unknown.

from foscommentbundle.

Hello,
Has this problem still unsolved yet?

I've also same problem in ->orderBy('c.ancesstors', 'ASC') in findCommentsByThread method .
Then i changed column to id instead of ancesstors.

from foscommentbundle.