Make WordPress Core

Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#8019 closed defect (bug) (fixed)

WXR import incorrectly handles comment ID's, Results in threading being lost

Reported by: dd32's profile DD32 Owned by: joostdevalk's profile joostdevalk
Milestone: 2.9 Priority: high
Severity: major Version: 2.7
Component: Import Keywords: needs-testing
Focuses: Cc:

Description

The Export'd WXR files & Import process contains the original Comment ID, and Comment Parent ID allready. However, Upon import, the Comment ID's are re-assigned, however the parent ID's are not.

The result is that the comment threading is lost entirely.

Eg: Previously comment 1234 was in reply to comment 1222, however, comment 1222 is now comment 22, but comment 1234 still has '1222' as its parent_id.

2 options are possible:

  • Remapping the comment parent ID's
  • Preserving the comment ID's

The only issue i see with preserving the comment ID's is that importing into another blog with comments allready could cause conflicts, which would require remapping of some comment ID's regardless.

Attachments (3)

wordpress.php.diff (3.5 KB) - added by joostdevalk 14 years ago.
Patch
testwxr.xml (9.7 KB) - added by joostdevalk 14 years ago.
WXR File I used to test the patch
8019.diff (3.6 KB) - added by ryan 14 years ago.

Download all attachments as: .zip

Change History (26)

#1 @ryan
15 years ago

[9100] is how we fixed this for posts.

#2 @ryan
15 years ago

  • Keywords early added
  • Milestone changed from 2.7 to 2.8

Too big a job for 2.7. Let's shoot for early 2.8 with a possible backport to a 2.7 dot release.

#3 @janeforshort
14 years ago

  • Milestone changed from 2.8 to Future Release

Punting due to feature freeze. Reconsider with next release.

#4 @tott
14 years ago

I am currently testing a patch that preserves the comment_IDs where possible. This solves most of the issues. I will attach the patch after some more testing.

#5 @lloydbudd
14 years ago

  • Milestone changed from Future Release to 2.9
  • Owner set to tott
  • Priority changed from normal to high
  • Status changed from new to assigned

With threaded commenting in core with 2.7, now more threading in the wild. Raising priority. Depending on 2.8 release status and nature of the solution should consider getting in 2.8 .

@joostdevalk
14 years ago

Patch

#6 @joostdevalk
14 years ago

  • Keywords has-patch added; needs-patch early removed
  • Owner changed from tott to joostdevalk

@joostdevalk
14 years ago

WXR File I used to test the patch

#7 @joostdevalk
14 years ago

  • Keywords needs-testing added

#8 @joostdevalk
14 years ago

  • Milestone changed from 2.9 to 2.8

I'd like to get this in in 2.8, I think it's a pretty simple and solid patch which solves a major annoyance...

#9 @Denis-de-Bernardy
14 years ago

  • Keywords commit added

IRC discussion for reference:

[09:30] <jdevalk> basically threw all comments into an array, sort them by id, remapped the wxr id's to the new id's and attach the correct parent
[09:31] <jdevalk> also working on a plugin that let's you import wxr files that are on the server, so you're not capped by HTTP upload restraints
[09:31] <ddebernardy> I take it you tested before/after with your data already, no?
[09:31] <jdevalk> yes.
[09:33] <ddebernardy> patch's logic seems correct
[09:33] <jdevalk> yeah it's not too hard
[09:34] <ddebernardy> an alternative method, which might be more robust, could be re-key the comments array based on parents
[09:34] <ddebernardy> array ( comment_parent => comments )
[09:34] <ddebernardy> and then do a double foreach loop
[09:34] <jdevalk> not sure i follow? :)
[09:35] <ddebernardy> well, it would cover the case where a weirdo plays with his database
[09:35] <ddebernardy> and ends up with comment IDs in an order than hasn't much to do with the comments date or hierarchy
[09:35] <ddebernardy> but it's such an edge case that we can ignore it imo
[09:36] <jdevalk> yeah i think so too

Safe to commit imo.

#10 @Denis-de-Bernardy
14 years ago

  • Milestone changed from 2.8 to 2.9

can wait until 2.9

#11 @Denis-de-Bernardy
14 years ago

  • Milestone changed from 2.9 to 2.8

unless someone wants to try it...

#12 @Denis-de-Bernardy
14 years ago

  • Keywords early added
  • Milestone changed from 2.8 to 2.9

per discussion in IRC

#13 @Denis-de-Bernardy
14 years ago

  • Keywords needs-patch added; has-patch needs-testing commit early removed

let's give the double array approach a try, just in case.

http://core.trac.wordpress.org/ticket/8019#comment:9

#14 follow-up: @ocean90
14 years ago

I want to test the patch but I get an error on WP and WPMU 2.8.4:

Parse error: syntax error, unexpected T_VARIABLE, expecting T_FUNCTION in .../wordpress.php on line 529

Is there something new?
Would be nice if the bug can be fixed.

#15 in reply to: ↑ 14 @ocean90
14 years ago

Sorry, my mistake.

It works now. Thanks.

#16 follow-up: @michaelh
14 years ago

Assuming the correct advice was provided, this fix didn't solve the problem according to this Forum thread:

http://wordpress.org/support/topic/312843

#17 in reply to: ↑ 16 @michaelh
14 years ago

Replying to michaelh:

Assuming the correct advice was provided, this fix didn't solve the problem

Retracting that statement as the patch wordpress.php.diff does in fact fix the threaded comment problem at 2.8.4:

http://wordpress.org/support/topic/312843?replies=17#post-1225031

#18 follow-up: @azaozz
14 years ago

  • Keywords needs-testing added; needs-patch removed

Is this good to go?

#19 @Denis-de-Bernardy
14 years ago

if we make the assumption that they're tossed in, in the correct order, it worked fine a few months back.

there were some talks of using a double foreach loop in the event they weren't. but it's mostly an improvement over the current patch.

#20 in reply to: ↑ 18 @tott
14 years ago

Replying to azaozz:

Is this good to go?

I tested against current trunk. Works fine as import on an empty blog as well as when importing on a blog which already has post/comment ids provided in the import. This should cover most cases and all further items would be improvements to this patch.

@ryan
14 years ago

#21 @ryan
14 years ago

Refreshed patch that applies against trunk. Needs testing.

#22 @ryan
14 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12301]) Preserve parentage when importing comments. Props joostdevalk. fixes #8019

#23 @voyagerfan5761
14 years ago

  • Cc WordPress@… added

I assume that blogs which already have imported content will have to delete and re-import their comments to receive any benefits from this patch?

Note: See TracTickets for help on using tickets.