Would be helpful for unit tests

CommentFileSizeAuthor
#9 2151427-comment-const-9.patch38.35 KBandypost
#9 interdiff.txt756 bytesandypost
#7 2151427-comment-const-7.patch37.61 KBandypost
#2 2151427-comment-const-2.patch37.57 KBTR
#1 2151427-comment-const-1.patch38.07 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
2151427-comment-const-1.patch38.07 KB

here we go!

TR’s picture

TR CreditAttribution: TR commented
FileSize
2151427-comment-const-2.patch37.57 KB

This is a common-sense improvement. +1 for doing this.

However, some of the added "use Drupal\comment\CommentInterface;" statements are not needed. For example, in /core/modules/comment/lib/Drupal/comment/CommentStorageController.php, CommentInterface is already referenced directly in the code without error. The CommentInterface interface doesn't have to be "use"-d because it is in the same namespace as CommentStorageController.

The attached patch differs from #1 in that it removes the unneeded use statements. Nothing else has been changed.

royal121’s picture

royal121 CreditAttribution: royal121 commented

The patch in #2 applies cleanly. Thanks.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +API change

makes sense but is an api change.
there is at least one change-notice that will need updating too.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Assigned: Unassigned » alexpott

Assigning to Alex about changes, if the change rejected the backup is mark deprecated
PS: there's no conclusion still according #1847540-7: [META] Clean up comment module tests and decouple from node

+++ b/core/modules/comment/comment.module
@@ -20,16 +20,6 @@
- * Comment is awaiting approval.
- */
-const COMMENT_NOT_PUBLISHED = 0;
-
-/**
- * Comment is published.
- */
-const COMMENT_PUBLISHED = 1;
-
-/**

This constants could be marked deprecated to avoid API change tag

Dries’s picture

Dries CreditAttribution: Dries commented

This makes sense to me.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
2151427-comment-const-7.patch37.61 KB

re-roll after #2061899: Remove references to global $user in Comment module

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2151427-comment-const-7.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt756 bytes
2151427-comment-const-9.patch38.35 KB

And another new entry after #1966448: comment_entity_load() breaks after disabling the Forum module
No other changes so back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Convert COMMENT_NOT_PUBLISHED & COMMENT_PUBLISHED to a constant on the comment interface » Change notice: Convert COMMENT_NOT_PUBLISHED & COMMENT_PUBLISHED to a constant on the comment interface
Assigned: alexpott » Unassigned
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change, +Needs change record

Committed be70033 and pushed to 8.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
Issue tags: -Needs change record

Filed change notice https://drupal.org/node/2156155

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Change notice: Convert COMMENT_NOT_PUBLISHED & COMMENT_PUBLISHED to a constant on the comment interface » Convert COMMENT_NOT_PUBLISHED & COMMENT_PUBLISHED to a constant on the comment interface
Priority: Major » Normal
Status: Needs review » Fixed

Looks fine to me, thanks!

Status: Fixed » Closed (fixed)

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