Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface

What I still don't understand, how can entities opt out of validation with this patch? How can rest module stop exposing content_moderation entities ... just this patch doesn't solve that bit :)

And even though the ContentModerationState entity type is internal, why wouldn't it support validation?

We had this discussion before in #2843753: Prevent ContentModerationState from being exposed by REST's EntityResource ... its a purely internal entity.

REST module should exclude that internal entity type in \Drupal\rest\Plugin\Deriver\EntityDeriver::getDerivativeDefinitions().

Fair, I guess content_moderation could exclude itself using some alter hook, if really needed.

One question about \Drupal\Core\Entity\FieldableEntityInterface::isValidationRequired ... why is this not used at all in Drupal core? Is it just me that this smells like another bug?

Thank you @amateesc for the explanation this makes sense.

I'm calling out to @tim.plunkett whether he believes that adding these 3 methods to ConfigEntityInterface makes sense.

At least for me personally I like that config entities are free of typed data and everything typed data related is orthogonal.

I made a patch on #2300677-200: JSON:API POST/PATCH support for fully validatable config entities that takes this patch and applies it to that issue.

I like the way it simplifies that logic. I add the trait there to the ConfigTest entity and made a trait for basic validation that implements the methods in ValidatableInterface that was basically does what we were doing in ConfigEntityResource. So basically if an ConfigEntity want the basic validation of typed data they can just implement the interface and use the trait.

Also before this idea in #2300677: JSON:API POST/PATCH support for fully validatable config entities we were only validating in the REST resource so if so it would not be obvious for a developer how to validate a config entity if they were making one programmatically.

Fair, I guess content_moderation could exclude itself using some alter hook, if really needed.

It already does this: content_moderation_rest_resource_alter().

why is this not used at all in Drupal core? Is it just me that this smells like another bug?

😱

I'm calling out to @tim.plunkett whether he believes that adding these 3 methods to ConfigEntityInterface makes sense.

@amateescu said in the other issue that we'd add this on a per-config entity type basis. In other words: to those config entity types that add support for validation. So for example class NodeType extends ConfigEntityBundleBase implements NodeTypeInterface would change to class NodeType extends ConfigEntityBundleBase implements NodeTypeInterface, ValidatableInterface (EDIT d'oh that's the example @amateescu gave in #8, sorry :P

Also before this idea in #2300677: JSON:API POST/PATCH support for fully validatable config entities for config entity types that support validation we were only validating in the REST resource so if so it would not be obvious for a developer how to validate a config entity if they were making one programmatically.

Great point! This enhances the PHP DX.

Well I'm still not convinced of putting typed data level stuff on config entities, but it seems to be nobody gives a shit.

I didn't mean to bypass you, Daniel. I thought you had just misinterpreted. Probably means I'm missing something. Moving back to prior state.

#2300677: JSON:API POST/PATCH support for fully validatable config entities is major, this is a blocker, so bumping this to major too.

I mostly try to keep things maintainable.

As of now config schema for example is not required for config entities to work, but rather are departed completely.
At least for me validation is just another level of config schema, also based upon typed data, but this time instead based upon constraints.

Don't get me wrong, there are advantages of putting stuff onto this interface as well.

@dawehner is you concern that with #2300677-184: JSON:API POST/PATCH support for fully validatable config entities #184 and before the typed data validation was something that happened on the Config REST resource level, i.e if you were using REST then typed data validation is not connected to Config entities....

But with this patch for any Config Entity that implements that new typed data validation will directly become part of the Entity itself?

But with this patch for any Config Entity that implements that new typed data validation will directly become part of the Entity itself?

But with this patch for any Config Entity that implements that new typed data validation will directly become part of the Entity itself?

Yeah basically. I think asking some of the configentity maintainers would be fair, given they can judge this better.

Maybe the answer will be: Its okay to add this one but limit to any further change.

One general question, is there a reason why we could not do this issue after the rest config issue? Could we introduce some other temporary way to flag config entites as validatable, but don't make this a public flag yet?

I read through #7, and didn't have to comment. But my question to Wim in slack was "why isn't config schema sufficient?"
Great to see @dawehner with similar thoughts.

I misunderstood the scope of this issue. But mostly because I kept reading the last patch in #2300677: JSON:API POST/PATCH support for fully validatable config entities, which then starts applying it to config entities.

So, in a vacuum this patch is fine? But I'm very wary of the other issue.

What should be the next step? Implement one-off validation code for each config entity?

This is a weird question ... I hope we don't suggest people to validate on their own, instead of using our systems.

Let's just get this in, people seemed to not be convinced by my argument, cool.

#25 isn't really convincing as an RTBC; please only RTBC things that you actually are signing off on as a peer reviewer. :)

So both @dawehner and @tim.plunkett have raised concerns about the longterm roadmap, that this might be used to add additional validation to config entity types other than their schemata. I understand that this is not being added to ConfigEntityInterface or anything and not added to any config entity types yet, that it's only an option, etc. However, I think the concerns are legitimate.

The concerns are:

1. That config schema is already the way we validate config, and so implementing other validation could be superfluous or even dangerous.
2. That config is not currently coupled to typed data and we want to keep it that way.
3. That implementing such other validation inconsistently for some config entity types on a case-by-case basis or as a gradual API addition might actually be worse.

I had all the same thoughts as I looked at this issue, and I'm not convinced by the arguments that they shouldn't be concerns. Like @tim.plunkett, I think this patch is fine as an improvement for the content entity API. Nice, even, as @tedbow says. So I think for all of us, the concerns are with #2300677: JSON:API POST/PATCH support for fully validatable config entities and not with this issue;. I'd like to see the relevant discussion of those concerns added to #2300677: JSON:API POST/PATCH support for fully validatable config entities before we proceed here. I'll also tag that issue for subsystem and framework manager review.

Also tagging this one for FM review since that's where we escalate when subsystem maintainers disagree, but let's please have an actual RTBC and attempt at consensus (or documenting the lack of consensus and counterpoints in the IS) before we do. :)

In case we decide to do this, we should file a follow up which does the following: It implements the validation inside a trait. This way we don't add the validation onto ConfigEntityBase, given it would show the wrong signal.

IMO ValidatableInterface is too broad better to prepend it kinda EntityFieldValidatationInterface

I really think #1818574: Support config entities in typed data EntityAdapter should be added before adding an interface and pretending to support customisability, where we actually don't support some.

Ok, let's block this on #1818574: Support config entities in typed data EntityAdapter.

As of #2300677-242: JSON:API POST/PATCH support for fully validatable config entities, this is no longer a blocker for that issue.

Restoring what I think was an unintentional tag name change in #34.

Should have been unpostponed 5 years ago, whoops

This blocks #2300677: JSON:API POST/PATCH support for fully validatable config entities.

IMHO we should also do #2327883-108: Field [storage] config have incomplete settings until they are saved here, because without it, we won't be able to actually validate some config entities! 😱 (Which is the goal stated in the issue summary.)

Quoting #2327883-108:

One thing I definitely hadn't thought of yet so far is how config entity objects may have different values in their runtime state vs their storage state, thanks to \Drupal\Core\Config\Entity\ConfigEntityStorage::mapToStorageRecord()! 😳 Fortunately, only 3 classes in core override it:

1. \Drupal\Core\Field\FieldConfigStorageBase::mapToStorageRecord()
2. \Drupal\field\FieldStorageConfigStorage::mapToStorageRecord()
3. \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplayStorage::mapToStorageRecord()

… plus 6 in contrib, 5 of which are virtually identical to core's Layout Builder one.

This means that for config entity validation to work, the infrastructure that provides validation for config entities must also be able to call mapToStorageRecord() not just prior to save, but prior to validation. But that method is currently protected 😅 So we need some way to get config entities into the same state as just before saving. We may want to do that as part of #2907163: Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface or in another issue.

@amateescu, do you have thoughts on how to do that? 🤓🙏

Also, #3324150: Add validation constraints to config_entity.dependencies introduced \Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::assertValidationErrors() which does:

  protected function assertValidationErrors(array $expected_messages): void {
    /** @var \Drupal\Core\TypedData\TypedDataManagerInterface $typed_data */
    $typed_data = $this->container->get('typed_data_manager');
    $definition = $typed_data->createDataDefinition('entity:' . $this->entity->getEntityTypeId());
    $violations = $typed_data->create($definition, $this->entity)->validate();

We will may need to update that for #48.

Do we really still need this if #1818574: Support config entities in typed data EntityAdapter already made this possible? This code was committed to Drupal core in 2018:

  public function testValidate() {
    $adapter = ConfigEntityAdapter::createFromEntity($this->entity);
    $violations = $adapter->validate();

Seems this is just a nice-to-have now?

Not to mention: not every config entity type will actually be validatable (we can guarantee this in core, not in contrib), so the existing ::isValidationRequired() is insufficient. We'd also need ::isValidationSupported() or something like that?

Demoting priority.

It would still be really useful and a DX improvement if all entities, whether config or content, had the same API for validation.