Comment status field should match node status field [#237636]

I just spent way too much time totally confused about why adding "status = 1" to a query to exclude unpublished comments wasn't working. Turns out that comments are backwards! Nodes are status = 1 = published which makes sense. Being published seems like the "norm" case, which makes sense with "1" even though the field is called "status" and not "published" so it's ambiguous. But having "status = 0" be published just sounds wrong and it's also the opposite if you're used to nodes. So I'm proposing it be changed so that "status = 1" is published the same as it is with nodes.

Michelle

Comment	File	Size	Author
#9	comments.publish.patch	2.05 KB	marcingy
#9
#7	comments.publish.patch	2.18 KB	marcingy
#7
#4	comments.publish.patch	2.12 KB	marcingy
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 23 March 2008 at 01:01

Yes. This is the tip of the ice berg among other comment/node API inconsistencies. Subscribing to this issue, in the hopes that once SoC is under way, I can work on fixing this up.

Comment #2

catch

he/him

English

CreditAttribution: catch commented 3 July 2008 at 12:00

Would be nice to fix this, subscribing.

Comment #3

Nick Lewis CreditAttribution: Nick Lewis commented 22 July 2008 at 05:13

Haha. Yeah -- there is really no excuse for it. Would love to hear why status = 1 actually means "unpublished" for comments.
A patch would probably look like:
-comment.install update reversing the status on each comment.
-find/ (informed) replace of every instance of $comment->status (who knows what uses that...)
-change documented in the update guide.

Seems easy enough. Never thought about the word "status" in the db table. But, it is nonsensical when you think about it:
-"Published content has *status*; drafts have *no status*.
Whaa?

Comment #4

marcingy CreditAttribution: marcingy commented 2 August 2008 at 03:20

Status:

Active

» Needs review

File	Size
comments.publish.patch	2.12 KB

Initial stab at a patch. Have the feeling that message in the install file may need to be changed to provide better feed back.

Comment #5

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 2 August 2008 at 04:22

This is a minor, minor point, but could we re-order the constant declarations in comment.module so that 0 comes first, then 1?

Comment #6

marcingy CreditAttribution: marcingy commented 2 August 2008 at 04:43

I agree and also the schema comments for status in the install file should also be reordered. I'll reroll tomorrow morning or later tonight.

Comment #7

marcingy CreditAttribution: marcingy commented 2 August 2008 at 05:14

File	Size
comments.publish.patch	2.18 KB

Reroll of patch with correct ordering of constants and schema comments

Comment #8

Dries CreditAttribution: Dries commented 2 August 2008 at 06:18

Status:

Needs review

» Needs work

I agree that this needs to be fixed. I think we can and should improve the update function though -- on Drupal.org it would take a really long time to execute that function and it might timeout.

I would propose the following 3 update queries:

1. update ... set status = 42 where status = 0
2. update ... set status = 0 where status = 1
3. update ... set status = 1 where status = 42

Comment #9

marcingy CreditAttribution: marcingy commented 2 August 2008 at 18:54

Status:

Needs work

» Needs review

File	Size
comments.publish.patch	2.05 KB

Amended update function as per Dries suggestion so as we just directly update the database.

Comment #10

Dries CreditAttribution: Dries commented 2 August 2008 at 20:31

Status:

Needs review

» Needs work

I've tested the upgrade path, ran all the simpletests to confirm that there are no regressions and committed this patch to CVS HEAD.

I'm marking this as 'code needs work' until we updated the upgrade documentation in the Drupal handbook. Please mark this fixed after we documented this API change.

Thanks marcingy!