Problem/Motivation

Part of #3518993: [META] Replace comment constants with enums

Steps to reproduce

Proposed resolution

CommentItemInterface - HIDDEN, CLOSED and OPEN moves to a CommentingStatus enum with cases Hidden, Closed and Open.

Remaining tasks

The 11.x MR needs review. It's a cherry pick of the commit to main, plus a regenerated baseline to ignore all the calls to deprecated constants from deprecated modules that have been split to contrib already.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#16 3547353-nr-bot_u0jkxy7g.txt91 bytesneeds-review-queue-bot
#11 3547353-nr-bot_1evv7c_a.txt91 bytesneeds-review-queue-bot
#8 3547353-nr-bot_ty41lo09.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3547353

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !13265

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Sorry to be that person

Did a quick search for CommentItemInterface::HIDDEN CommentItemInterface::OPEN and CommentItemInterface::CLOSED

found CommentItem::isEmpty (which also has OPEN and CLOSED)

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

No need to apologise, thanks for finding that.

mstrelan’s picture

mstrelan commented
Title: Move HIDDEN, CLOSED and OPEN constants in CommentInterface to new CommentingStatus enum » Move HIDDEN, CLOSED and OPEN constants in CommentItemInterface to new CommentingStatus enum
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Appeared to be the last reference

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3547353-nr-bot_ty41lo09.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Rebased and replicated changes from CommentLinkBuilderTest to the new HistoryCommentLinkBuilderTest

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good rebase

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3547353-nr-bot_1evv7c_a.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community

Rebased and resolved conflicts due to #3513120: Fix LongLineDeclaration in Functional tests

astonvictor’s picture

astonvictor commented

It works for me, but the only question: Should we use old names (HIDDEN, CLOSED, and OPEN - all uppercase)?

mstrelan’s picture

mstrelan commented

No, this has already been through the coding style review process and we use PascalCase for enums.

astonvictor’s picture

astonvictor commented

ok, thanks for the response
+1 RTBC

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3547353-nr-bot_u0jkxy7g.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Rebased, back to NR as I had to manually fix up some conflicts

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems was previously RTBC before the bot

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

NW for merge conflict.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community

Rebased. There was a conflict with comment_preview being moved from comment.module to CommentForm::preview so I reapplied one change there. The other conflict was because core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php no longer exists.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

This looks close.

Another merge conflict for added return type on CommentTestTrait::addDefaultCommentField.

Also MR suggestions to bump the deprecation version.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Rebased, applied suggestions and updated the version in the CR.

godotislate closed merge request !13265

  • godotislate committed 19163c55 on main 
    feat: #3547353 Move HIDDEN, CLOSED and OPEN constants in...

  • godotislate committed 3c489128 on 11.x 
    feat: #3547353 Move HIDDEN, CLOSED and OPEN constants in...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Needs review » Fixed

Committed 19163c5 and pushed to main and 3c48912 and pushed to 11.x. Thanks!

Made a small edit to the CR and published.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • godotislate committed a9d79e3b on 11.x 
    Revert "feat: #3547353 Move HIDDEN, CLOSED and OPEN constants in...
godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Patch (to be ported)

Reverted 11.x for PHPStan errors. Needs a separate MR.

godotislate’s picture

godotislate
he/him
commented

Re #30, looks like deprecation warnings
https://git.drupalcode.org/project/drupal/-/jobs/8860687

------ ------------------------------------------------------------- 
  Line   core/modules/history/src/HistoryCommentLinkBuilder.php       
 ------ ------------------------------------------------------------- 
  53     Fetching deprecated class constant HIDDEN of interface       
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:  
         in drupal:11.4.0 and is removed from drupal:13.0.0.          
           Use \Drupal\comment\CommentingStatus::Hidden instead.      
         🪪  classConstant.deprecated                                 
 ------ ------------------------------------------------------------- 
 ------ ------------------------------------------------------------------- 
  Line   core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php  
 ------ ------------------------------------------------------------------- 
  192    Fetching deprecated class constant OPEN of interface               
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Open instead.              
         🪪  classConstant.deprecated                                       
  201    Fetching deprecated class constant OPEN of interface               
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Open instead.              
         🪪  classConstant.deprecated                                       
  219    Fetching deprecated class constant OPEN of interface               
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Open instead.              
         🪪  classConstant.deprecated                                       
  242    Fetching deprecated class constant CLOSED of interface             
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Closed instead.            
         🪪  classConstant.deprecated                                       
  245    Fetching deprecated class constant CLOSED of interface             
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Closed instead.            
         🪪  classConstant.deprecated                                       
  248    Fetching deprecated class constant CLOSED of interface             
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Closed instead.            
         🪪  classConstant.deprecated                                       
  251    Fetching deprecated class constant OPEN of interface               
         Drupal\comment\Plugin\Field\FieldType\CommentItemInterface:        
         in drupal:11.4.0 and is removed from drupal:13.0.0.                
           Use \Drupal\comment\CommentingStatus::Open instead.              
         🪪  classConstant.deprecated                                       
 ------ -------------------------------------------------------------------
mstrelan’s picture

mstrelan commented

I think we should add those to the baseline for 11.x rather than fixing them, since those modules will be removed. I believe they were fixed in earlier iterations of the MR before removal.

godotislate’s picture

godotislate
he/him
commented

That sounds fine. Actually, I think the baseline can be downloaded from the failed job artifacts: https://git.drupalcode.org/project/drupal/-/jobs/8860687/artifacts/file/...

mstrelan opened merge request !15053

mstrelan’s picture

mstrelan commented

I opened an MR with the baseline added, but I'm not sure that'll work, I think the phpunit jobs might fail if the deprecations are still triggered. But I don't seem to have access to even view the failed job, so I can't really tell. I could put up another MR that fixes the deprecations in 11.x, but that would be updating modules that have already been split to contrib, so it seems weird.

godotislate’s picture

godotislate
he/him
commented

Strangely I could not view the downstream job either, but I could re-run the parent job. So I did, and it passed.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Patch (to be ported) » Needs review

OK great then I guess the 11.x MR needs review. It's a cherry pick of the commit to main, plus a regenerated baseline to ignore all the calls to deprecated constants from deprecated modules that have been split to contrib already.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good backport.

  • godotislate committed 979d45e3 on 11.x 
    refactor: #3547353 Move HIDDEN, CLOSED and OPEN constants in...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 979d45e and pushed to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

godotislate closed merge request !15053

Status: Fixed » Closed (fixed)

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