Problem/Motivation

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

Steps to reproduce

N/A

Proposed resolution

CommentInterface - ANONYMOUS_MAYNOT_CONTACT, ANONYMOUS_MAY_CONTACT and ANONYMOUS_MUST_CONTACT moves to a AnonymousContact enum with cases Forbidden, Allowed and Required.

Remaining tasks

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

Issue fork drupal-3547349

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 !13263

mstrelan’s picture

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

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Pretty simple to review. Applied the MR and did a search for ANONYMOUS_MAY_CONTACT, ANONYMOUS_MAY_CONTACT, and ANONYMOUS_MAYNOT_CONTACT and all instances appear to be replaced.

LGTM

oily’s picture

oily commented

Reviewed the code. Found no issues with the code. Added a review/ comment regarding the @todo.

smustgrave’s picture

smustgrave commented

Have to ask why review an issue just reviewed? Seen it before and always curious. More reviews the better but wanted to ask.

oily’s picture

oily commented

Ran test-only script:

PHPUnit 11.5.39 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-3547349/core/phpunit.xml.dist
E.                                                                  2 / 2 (100%)
Time: 00:01.967, Memory: 8.00 MB
There was 1 error:
1) Drupal\Tests\comment\Kernel\CommentValidationTest::testValidation
Error: Class "Drupal\comment\AnonymousContact" not found
/builds/issue/drupal-3547349/core/modules/comment/tests/src/Kernel/CommentValidationTest.php:146
ERRORS!
Tests: 2, Assertions: 58, Errors: 1, PHPUnit Deprecations: 3.
Exiting with EXIT_CODE=2

I think that is exactly as required?

smustgrave’s picture

smustgrave commented

this was a task not a bug or feature so test-only run wasn't needed actually

oily’s picture

oily commented

@smustgrave I admit I have targeted RTBC issues too much. But this time only added a review/ comment whereas before I updated code. My comment offers to complete a @todo. Seeing @mstrelan has done a lot of quality code was wondering if the @todo is out of scope or if I could complete it. Wondered if he had felt he had done enough (which he has). I have coded enum's in my time and am keen to put icing on the top if in scope.

The 'task' not 'bug' point is taken. I live and learn.

smustgrave’s picture

smustgrave commented

Running it doesnt hurt anything

mstrelan’s picture

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

Needs work for #5, feel free to work on that.

Re: tests-only, the way it works is to do get a diff of any tests, apply it to 11.x / HEAD and re-run the tests. In this case the enum we've introduced doesn't exist in HEAD so of course it fails. As @smustgrave said, it's usually for bugs, which usually don't need new classes.

oily’s picture

oily commented

@mstrelan Read #11 with interest. Sounds good.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

I took a careful review of this.

All replacements match and logic seems unchanged.
Deprecations look correct.

I'm not sure if the return type or new parameter types are ok.

CR looks good as well.

I did not pull this down and search but that was done in 4.

Only open question is about the types I'll try to track down an answer.

dcam’s picture

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

Following on from the review in #14, I'm guessing that the question at hand is whether it's OK to type hint parameters or return values with an enum, but if I'm mistaken then please let me know. I checked the @param and @return data types and researched the answers.

The enum documentation page shows an example of a function parameter typed hinted with an enum.

The ::tryFrom() docs themselves show a return type of static|null, so it's OK to type hint a return value with an enum.

They look good to me. I'm going to RTBC this.

mstrelan’s picture

mstrelan commented

I think what #14 was referring to are these two existing signatures changing:

protected function getAnonymousContactDetailsSetting(CommentInterface $comment): AnonymousContact
protected function setCommentAnonymous(AnonymousContact|int $level)

Which is really only an issue if someone is extending the classes and overriding these functions, or passing something other than an int to setCommentAnonymous.

dcam’s picture

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

My mistake.

I searched contrib for uses of both functions and turned up one result. RDF extends CommentTestBase and uses setCommentAnonymous(), which makes sense because it was in Core. See https://git.drupalcode.org/project/rdf/-/blob/2.x/tests/src/Functional/C....

mstrelan’s picture

mstrelan commented

Thanks for finding that. Since it's passing an int and we have provided a union type then this should be fine.

dcam’s picture

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

Yeah. Running the RDF test with these changes triggered the deprecation warning as it should. So I'll set this to RTBC.

oily’s picture

oily commented
Issue summary: View changes

I agree with the RTBTC. Checked the CR. Straightforward, looks good.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Too late for 11.3.0, deprecations need updating to 11.4.0 for removal in 13.

dcam’s picture

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

The deprecation version has been updated. I'm re-RTBCing it because this is a minor change.

quietone’s picture

quietone commented

I updated credit and reviewed the Change record, which is fine. Leaving at RTBC

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.

  • catch committed 5df9e232 on 11.x 
    task: #3547349 Move ANONYMOUS_ constants in CommentInterface to new...

  • catch committed 5c1021d1 on main 
    task: #3547349 Move ANONYMOUS_ constants in CommentInterface to new...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and 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.

catch closed merge request !13263

Status: Fixed » Closed (fixed)

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