Problem/Motivation

Part of #2851705: [meta] Deprecate DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED - we want to deprecate DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED. Doing this in a single issue is too hard to review and we want to change the API as we go.

This ticket is for the Comment module.

Proposed resolution

- Add CommentPreview enum
- Replace usages of constants with new enum
- Update $mode param in CommentTestBase::setCommentPreview to take CommentPreviewMode enum, throw a deprecation when its an integer.
- $message is unused by setCommentSettings so we can remove $mode_text entirely. We can used named params to continue to pass $field_name. See #3478418: Deprecate $message param from \Drupal\Tests\comment\Functional\CommentTestBase::setCommentSettings
- Pass $mode->value into setCommentSettings

API changes

CommentTestBase::setCommentPreview now takes a CommentPreviewMode enum instead of int for $mode

Release notes snippet

N/A

Issue fork drupal-3538660

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:

Comments

acbramley created an issue. See original summary.

acbramley’s picture

Issue summary: View changes

acbramley’s picture

Status: Active » Needs review
mstrelan’s picture

Status: Needs review » Needs work

Looks great, only 3 minor comments

acbramley’s picture

Status: Needs work » Needs review
mstrelan’s picture

Status: Needs review » Reviewed & tested by the community

Happy with that, will leave to a committer to decide on BC implications.

dww’s picture

Tentatively agree this is RTBC, although I couldn't resolve the 2 open threads in good faith, and I even opened a 3rd one.

Everything else in the MR looks good to me. Very straight-forward conversion. Made a few tiny edits to the CR for formatting, but that also looks good and is clear.

dww’s picture

Saving credits.

catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks good - I responded to and closed the open threads on the MR because in each case they didn't need any changes for me.

Committed/pushed to 11.x, thanks!

catch’s picture

  • catch committed 2f67dfc5 on 11.x
    Issue #3538660 by acbramley, mstrelan, dww: Add CommentPreview enum and...
catch’s picture

Status: Fixed » Closed (fixed)

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