Add validation constraints to comment.type.*

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

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

MR appears to have phpcs errors.

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.

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

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

I don't get why the id cannot be string and this test fails. Trying to figure why that would not be allowed. The schema does tell us string is fine, is that wrong then?

Drupal\Tests\comment\Kernel\CommentStringIdEntitiesTest::testCommentFieldNonStringId
Failed asserting that exception of type "Drupal\Core\Config\Schema\SchemaIncompleteException" matches expected exception "UnexpectedValueException". Message was: "Schema errors for comment.type.foo with the following errors: 0 [target_entity_type_id] The 'entity_test_string_id' entity is not commentable" at
core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
vendor/symfony/event-dispatcher/EventDispatcher.php:206
vendor/symfony/event-dispatcher/EventDispatcher.php:56
core/lib/Drupal/Core/Config/Config.php:230
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:260
core/lib/Drupal/Core/Entity/EntityStorageBase.php:487
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:239
core/lib/Drupal/Core/Entity/EntityBase.php:370
core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:618
core/modules/comment/tests/src/Kernel/CommentStringIdEntitiesTest.php: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

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.

All green.

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

Before

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

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.

Conflict was on cspell word list. Rebased, keeping rtbc

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.

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

Added CR

Added some nits to the MR.

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.

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

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

Unrealted failure, all good imo.