Please, find attached a patch to resolve a bug (probably a cut/paste from D7) on comment mode type detection.

Please review.

CommentFileSizeAuthor
#10 permalink.get_display_ordinal.patch1.26 KBpaolomainardi
#9 permalink.get_display_ordinal.patch2.89 KBpaolomainardi
permalink.get_display_ordinal.patch1.63 KBpaolomainardi

Comments

paolomainardi’s picture

paolomainardi commented

Any chance to get this reviewed ?

paolomainardi’s picture

paolomainardi commented

Anyone is using comment redirecting please be warned that there are some errors on backporting that not permits to redirect well, use this patch if you have some problems.

todd nienkerk’s picture

todd nienkerk commented
Assigned: Unassigned » todd nienkerk

Thanks for the patch. I'll review it shortly.

Quick question:

probably a cut/paste from D7

Do you mean D5?

todd nienkerk’s picture

todd nienkerk commented
Status: Active » Needs review
paolomainardi’s picture

paolomainardi commented

Dear Todd,

No i mean D7, if i'm not wrong this functinos are backported by D7 right ? Because they are really similar.

todd nienkerk’s picture

todd nienkerk commented

paolomainardi: You're absolutely right. D5 and D6 use COMMENT_MODE values of 1-4; D7 uses 0-1. This looks like a good fix.

Would you mind re-rolling the patch without the debugging code added? (See the first group of changes in the patch you submitted.)

todd nienkerk’s picture

todd nienkerk commented
Title: [PATCH] Bug on "permalink_comment_get_display_ordinal" comment type detection » Bug: permalink_comment_get_display_ordinal() comment mode detection not fully backported to D6
Status: Needs review » Needs work
paolomainardi’s picture

paolomainardi commented

Todd, yes, i'll send you the patch as soon as possible later in the day.

Thanks.

paolomainardi’s picture

paolomainardi commented
StatusFileSize
new permalink.get_display_ordinal.patch2.89 KB

Updated patch without debug code.

paolomainardi’s picture

paolomainardi commented
StatusFileSize
new permalink.get_display_ordinal.patch1.26 KB

Sorry, in attachment the correct one.

todd nienkerk’s picture

todd nienkerk commented
Status: Needs work » Needs review
Issue tags: -permalink

I've committed this patch to the dev branch. Can you help review it?

paolomainardi’s picture

paolomainardi commented

Dear Todd, yes, give some days and i'll give you my results.

Thanks a lot to commit.

paolomainardi’s picture

paolomainardi commented

Dear Todd, do you still need help to review this patch ?

JoeMcGuire’s picture

JoeMcGuire commented
Priority: Normal » Critical
Status: Needs review » Needs work

Looks good fixed permalinks for flat list comments.

#10 Logic is working - might consider using the constants

+  if ($mode == 1 || $mode == 2) {

to be

+  if ($mode == COMMENT_MODE_FLAT_COLLAPSED || $mode == COMMENT_MODE_FLAT_EXPANDED) {

Patches should also be made from the modules root rather than drupals. Nice work.

Changed to critical as this module is on sites using flat lists and those permalinks are broken.