Add validation constraints to comment.type.* [#3455066]

Comment type entity is not yet fully validatable:

./vendor/bin/drush config:inspect --filter-keys=comment.type.comment --detail --list-constraints --fields=key,validatability,constraints

>  🤖 Analyzing…

 ------------------------------------------------- ------------- ---------------------------------------------------------------------- 
  Key                                               Validatable   Validation constraints                                                
 ------------------------------------------------- ------------- ---------------------------------------------------------------------- 
  comment.type.comment                              91%           ValidKeys: '<infer>'                                                  
   comment.type.comment:                            Validatable   ValidKeys: '<infer>'                                                  
   comment.type.comment:_core                       Validatable   ValidKeys:                                                            
                                                                    - default_config_hash                                               
   comment.type.comment:_core.default_config_hash   Validatable   NotNull: {  }                                                         
                                                                  Regex: '/^[a-zA-Z0-9\-_]+$/'                                          
                                                                  Length: 43                                                            
                                                                  ↣ PrimitiveType: {  }                                                 
   comment.type.comment:dependencies                Validatable   ValidKeys: '<infer>'                                                  
   comment.type.comment:description                 Validatable   Regex:                                                                
                                                                    pattern: '/([^\PC\x09\x0a\x0d])/u'                                  
                                                                    match: false                                                        
                                                                    message: 'Text is not allowed to contain control characters, only   
                                                                  visible characters.'                                                  
                                                                  ↣ PrimitiveType: {  }                                                 
   comment.type.comment:id                          Validatable   Regex:                                                                
                                                                    pattern: '/^[a-z0-9_]+$/'                                           
                                                                    message: 'The %value machine name is not valid.'                    
                                                                  Length:                                                               
                                                                    max: 166                                                            
                                                                  ↣ PrimitiveType: {  }                                                 
   comment.type.comment:label                       Validatable   Regex:                                                                
                                                                    pattern: '/([^\PC])/u'                                              
                                                                    match: false                                                        
                                                                    message: 'Labels are not allowed to span multiple lines or contain  
                                                                  control characters.'                                                  
                                                                  NotBlank: {  }                                                        
                                                                  ↣ PrimitiveType: {  }                                                 
   comment.type.comment:langcode                    Validatable   NotNull: {  }                                                         
                                                                  Choice:                                                               
                                                                    callback:                                                           
                                                                  'Drupal\Core\TypedData\Plugin\DataType\LanguageReference::getAllVali  
                                                                  dLangcodes'                                                           
                                                                  ↣ PrimitiveType: {  }                                                 
   comment.type.comment:status                      Validatable   ↣ PrimitiveType: {  }                                                 
   comment.type.comment:target_entity_type_id       NOT           ⚠️  @todo Add validation constraints to config entity type:           
                                                                  comment.type.*                                                        
   comment.type.comment:uuid                        Validatable   Uuid: {  }                                                            
                                                                  ↣ PrimitiveType: {  }                                                 
 ------------------------------------------------- ------------- ----------------------------------------------------------------------

Steps to reproduce

Get a local git clone of Drupal core 11.x.
composer require drupal/config_inspector — or manually install https://www.drupal.org/project/config_inspector/releases/2.1.5 or newer (which supports Drupal 11!)
composer require drush/drush
vendor/bin/drush config:inspect --filter-keys=comment.type.comment --detail --list-constraints

Proposed resolution

Add validation constraints to missing properties.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

More validation 🚀

Release notes snippet

None.

Comment	File	Size	Author
#22	3455066-nr-bot.txt	90 bytes	needs-review-queue-bot
#20	3455066-nr-bot.txt	90 bytes	needs-review-queue-bot
#19	Screenshot 2025-03-03 at 11.42.34 AM.png	25.64 KB	smustgrave
#19	Screenshot 2025-03-03 at 11.26.37 AM.png	23.28 KB	smustgrave

Issue fork drupal-3455066

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

3455066-add-validation-constraints changes, plain diff MR !8568
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

17 June 2024 at 13:19

mtift created an issue. See original summary.

Comment #2

mtift

he/him

English

Minnesota, USA

commented 17 June 2024 at 13:21

Issue summary:

View changes

Comment #3

27 June 2024 at 12:34

borisson_ made their first commit to this issue’s fork.

Comment #4

mtift

he/him

English

Minnesota, USA

commented 27 June 2024 at 12:43

Assigned:

mtift

» Unassigned

Comment #5

borisson_

Dutch

Mechelen, 🇧🇪

commented 27 June 2024 at 12:58

This requires us to add something like EntityTypeExistsValidator, currently working on that.

Comment #6

28 June 2024 at 07:08

borisson_ opened merge request !8568

Comment #7

borisson_

Dutch

Mechelen, 🇧🇪

commented 28 June 2024 at 12:18

Spellcheck fails, but not sure how to better name this.

Comment #8

borisson_

Dutch

Mechelen, 🇧🇪

commented 28 June 2024 at 12:45

Status:	Active	» Needs review
Issue tags:		+DevDaysBurgas2024

Comment #9

smustgrave commented 28 June 2024 at 15:53

Status:

Needs review

» Needs work

MR appears to have phpcs errors.

Comment #10

pooja_sharma commented 28 June 2024 at 18:39

I have observed the test failures, may be we can use:

"commentAccepted/commentEligible/commentAllowed" in place of "Commentable"

As "commentAccepted/commentEligible/commentAllowed" word would be more closer/relative to the "Commentable" word to fix cspell test failures.

Left suggestion not updating MR, as most of the work done already.

Comment #11

23 February 2025 at 10:21

bbrala made their first commit to this issue’s fork.

Comment #12

bbrala

Netherlands

commented 23 February 2025 at 10:34

Status:

Needs work

» Needs review

Commentable makes so much sense i dont want to change it, just added to the drupal words. THink this can go back to NR

Comment #13

bbrala

Netherlands

commented 23 February 2025 at 10:49

Status:

Needs review

» Needs work

Seems a test is still fialing since it expects a string, not null.

Comment #14

bbrala

Netherlands

commented 23 February 2025 at 13:12

edit: Seems the message is more generic, when an entity doesnt use interger id, it doesn't validate as commentable. Which makes sense, the error is just not adjusted to that situation

Comment #15

bbrala

Netherlands

commented 23 February 2025 at 15:53

Ok, so the issue remaining is the fact that in ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing it tries to test what happens when a property is set to null. When EntityTypeExistsConstraintValidator is validated it also executes the code:

    if (!is_string($value)) {
      throw new UnexpectedTypeException($value, 'string');
    }

Which is then thrown. When looking at other implementations where this eror is thrown i see ContentLanguageSettings with EntityBundleExists. This sems to be an immutable property.

I'm now guessing there is 2 possible solutions:

1. The target_entity_type_id property of the comment type should be immutable.
2. It is not immutable, but instead of throwing an UnexpectedTypeException is might just need to add a validation error that the property shhould be string.

What the right solution is, i dont know. Hopefully someone does :x

Comment #16

bbrala

Netherlands

commented 25 February 2025 at 15:27

Did some digging. Ended up removing the exception and checking for null and '' in IsCommentable and EntityTypeExists. Seems to make more sense. Also adjusted the test so it expects extra validation errors when looking for null.

Comment #17

bbrala

Netherlands

commented 25 February 2025 at 19:54

Status:

Needs work

» Needs review

All green.

Comment #18

borisson_

Dutch

Mechelen, 🇧🇪

commented 25 February 2025 at 21:24

I think this looks good, there's no way I can rtbc this issue, because I wrote part of it.

Comment #19

smustgrave commented 3 March 2025 at 16:57

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

Status	File	Size
new	Screenshot 2025-03-03 at 11.26.37 AM.png	23.28 KB
new	Screenshot 2025-03-03 at 11.42.34 AM.png	25.64 KB

Before

before

After

after

"EntityTypeExists" We actually used a similar constraint in book contrib so glad to see this is going to be part of core.

Did a review of the code changes and nothing stands out and comments appear to be functioning just fine (11.x standard profile install locally).

LGTM

Comment #20

needs-review-queue-bot commented 6 March 2025 at 18:14

Status:

Reviewed & tested by the community

» Needs work

Status	File	Size
new	3455066-nr-bot.txt	90 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.

Comment #21

bbrala

Netherlands

commented 7 March 2025 at 07:29

Status:

Needs work

» Reviewed & tested by the community

Conflict was on cspell word list. Rebased, keeping rtbc

Comment #22

needs-review-queue-bot commented 14 March 2025 at 17:13

Status:

Reviewed & tested by the community

» Needs work

Status	File	Size
new	3455066-nr-bot.txt	90 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.

Comment #23

bbrala

Netherlands

commented 21 March 2025 at 14:41

Status:	Needs work	» Reviewed & tested by the community
Issue tags:		+no-needs-review-bot

This was a clean rebase. Not sure why its confused.

Comment #24

bbrala

Netherlands

commented 29 March 2025 at 10:20

Added CR

Comment #25

longwave

he/him

English

commented 16 April 2025 at 14:04

Status:

Reviewed & tested by the community

» Needs work

Added some nits to the MR.

Comment #26

bbrala

Netherlands

commented 16 April 2025 at 14:29

Fixed a few issues, but still need to make the constraint container aware to inject the entitty type manager and use that in the validator.

Comment #27

bbrala

Netherlands

commented 16 April 2025 at 14:36

Think nullanble is not allow, but willl cycle back later.

Comment #28

bbrala

Netherlands

commented 16 April 2025 at 15:11

Status:

Needs work

» Needs review

All green after cleanup. But to much to set rtbc right away

Comment #29

bbrala

Netherlands

commented 16 April 2025 at 15:21

Status:

Needs review

» Needs work

Comment #30

bbrala

Netherlands

commented 16 April 2025 at 15:33

Status:

Needs work

» Needs review

Unrealted failure, all good imo.

Comment #31

borisson_

Dutch

Mechelen, 🇧🇪

commented 25 April 2025 at 09:43

Status:

Needs review

» Reviewed & tested by the community

Looks like all the remarks by longwave have been resolved.

Comment #32

longwave

he/him

English

commented 19 June 2025 at 22:13

Status:

Reviewed & tested by the community

» Needs work

Looks good, one question about the placement of IsCommentableConstraint.

Comment #33

19 June 2025 at 22:13

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.

Add validation constraints to comment.type.*

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3455066

Comments

Change record drafts

Parent issue