This patch adds UUID support for Comments. It is pretty straightforward and similar in architecture to the other implementations.

CommentFileSizeAuthor
#5 889410_comments.patch4.27 KBgdd
#4 uuid_comments.patch3.53 KBrecidive
#3 889410_comments.patch4.02 KBgdd
#1 889410_comments.patch2.52 KBgdd

Comments

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
StatusFileSize
new 889410_comments.patch2.52 KB
recidive’s picture

recidive commented
Status: Needs review » Needs work

Sorry for the delay. I finally got time to review your patch.

The patch looks great but it's missing a setting for automatic UUID generation for comments. And a section in uuid_sync() for generating the missing UUIDs.

Could you please add those?

Thanks.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new 889410_comments.patch4.02 KB

Yes you're right, new patch attached (not really tested though, should be fine.)

recidive’s picture

recidive commented
StatusFileSize
new uuid_comments.patch3.53 KB

Hi, thanks for working on this.

I could find two hunks where it was relating to users not comments.

We really need someone to test this before commiting.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
StatusFileSize
new 889410_comments.patch4.27 KB

OK I did some testing of this today. I took a fresh D6 installation and used Devel to generate several nodes with comments. Then I installed UUID. I enabled generation of UUIDs for comments, then I ran the sync function for existing comments, and it worked fine, with one small bug which is fixed in the attached patch. Then I used Devel to generate several more nodes with comments and they all worked as well. I uninstalled and everything went away. So I'd say this is ready to go. I also tested upgrading from the existing version which worked.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

Oh in the attached patch I also made sure the new variable gets deleted on uninstall.

recidive’s picture

recidive commented
Status: Needs review » Fixed

Commited to CVS. Thanks!

I've also tagged a Beta 2 release that should be packed soon.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

I actually have several more patches to submit I think. I am looking into removing my own UUID implementation from Deploy and instead depending on this one, but I have a lot of functionality that needs to be transferred first.

recidive’s picture

recidive commented

Hi heyrocker, that sounds great!

Let me know if you want to be a co-maintainer of the project, if you think that would speed up the process, and makes sense for you.

Feel free to contact me directly if you want to.

Thanks.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

If you want to grant that it is fine, but I still want my patches to be reviewed since I'm still new to the module.

recidive’s picture

recidive commented

That's fine. I'm looking forward for applying your patches. Thanks!

Status: Fixed » Closed (fixed)

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