The DataDefinitionInterface is missing setConstraints method

Are you 100% sure about this? There is not a single setter defined on the DataDefinitionInterface

I just ran in to this. It makes sense. Besides the fact that \Drupal\Core\TypedData\DataDefinition::setConstraints already uses {@inheritdoc}, which could indicate that this was meant to be there all along, I found that the following interfaces contain a addConstraint() method:

• \Drupal\Component\Plugin\Context\ContextDefinitionInterface
• \Drupal\Core\Entity\EntityTypeInterface
• \Drupal\Core\Field\FieldConfigInterface
• \Drupal\Core\TypedData\DataDefinitionInterface

All these interfaces also contain a setConstraints() method, except \Drupal\Core\TypedData\DataDefinition. When having the option to add constraints, it makes sense to be able to add / override them.

Attached is a patch to add this.

Hope this comment gets the tests running...

Maybe a new upload...

I see nothing wrong here. Looks like a good, solid (and originally intended) addition, although I question whether adding methods to existing interfaces constitutes a BC break.

Ah yes, excellent point. I checked core, but we don't know what contrib is doing. As discussed on IRC with phenaproxima I fixed the doc block and added a todo.

I doubt that contrib is re-implementing DataDefinitionInterface, but who knows. In this case, better safe than sorry. I think this looks good.

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -271,7 +271,23 @@ public function getConstraint($constraint_name) {
+   * @todo Needs to be added to \Drupal\Core\TypedData\DataDefinitionInterface
+   *   as mentioned in https://www.drupal.org/node/2824979.

Let's create a proper @todo. i.e. File a new issue to do this and link it here. Also if we add it to DataDefinitionInterface we can remove it from \Drupal\Core\Field\FieldConfigInterface()

Also the issue summary and title need updating for the new fix.

I created #2852432: \Drupal\Core\TypedData\DataDefinition::setConstraints shouldn't use {@inheritdoc} to change the function doc comment for \Drupal\Core\TypedData\DataDefinition::setConstraints.

Attached is a new patch to fix this issue. I think the BC breaks means this will be postponed to 9.x.

1. +++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
   @@ -254,27 +254,4 @@ public function addPropertyConstraints($name, array $constraints);
   -   * NOTE: This will overwrite any previously set constraints. In most cases
   -   * FieldConfigInterface::addConstraint() should be used instead.
   -   *
   -   * Note that constraints added via this method are not stored in configuration
   -   * and as such need to be added at runtime using
   -   * hook_entity_bundle_field_info_alter().
   

   This documentation should move to be inlined in \Drupal\Core\Field\FieldConfigBase::setConstraints()

2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -186,6 +186,24 @@ public function getSetting($setting_name);
   +   * Sets the array of validation constraints for the FieldItemList.
   

   The field item list is not relevant here now.

3. Maybe one way around the Drupal 9 move is to introduce a ConstraintSomethingInterface or SomethingConstraintInterface... not sure what that something is atm... but it could have the following methods:
   • getConstraints()
   • getConstraint()
   • addConstraint()
   • setConstraints()

   And then both DataDefinition and ContextDefinitionInterface can implement/extend it. And the D9 issue can be to make DataDefinitionInterface extend the new interface. This way code can be written to be both D8 and D9 compatible if someone re-implements DataDefinition - though I'm not sure who is going to do that.

Adding stuff do an interface is not necessarily a BC break, see https://www.drupal.org/node/2562903/

I think given #15 going back to #7 is probably the best idea. I really don't think anyone is going to have re-implemented typed data in contrib - everyone is going to be extending from DataDefinition or FieldConfigBase. FieldConfigBase is the only other concrete class that implements DataDefinitionInterface without extending DataDefinition - and fortunately setConstraints is on FieldConfigInterface.

The remaining question is should we introduce an new interface to bring ContextDefinitionInterface and DataDefinitionInterface together?

Attached an updated patch addressing #14-1 and #14-2. Thanks dawehner for pointing out the BC policy!
Only remaining thing is possibly introducing a new interface as mentioned in #14-3. I guess this is a separate issue?

Moving back to 8.4.x and needs review..

Patch looks great to me and it seems to conform to all the feedback thus far.

The remaining question is should we introduce an new interface to bring ContextDefinitionInterface and DataDefinitionInterface together?

I'm +1 for this idea. How about calling it ConstraintAcceptingInterface?

+++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
@@ -528,6 +528,12 @@ public function getConfig($bundle) {
+   *
+   * Note that constraints added via this method are not stored in configuration
+   * and as such need to be added at runtime using
+   * hook_entity_bundle_field_info_alter().
+   *
+   * @see hook_entity_bundle_field_info_alter()
    */
   public function setConstraints(array $constraints) {
     $this->constraints = $constraints;

Adding docs to @inheritdoc is not supported by api.drupal.org (or the standard). These docs can be just inline to the implementation since they are totally about how this specific implementation is used.

There are hundreds of places in core adding docs to @inheritdoc. Was this policy added later? What if the added documentation is important for api.drupal.org, should you just add a complete doc block (and possibly duplicate docs) and not use {@inheritdoc}? Just being curious :)

Anyway, a updated patch is attached.

Looks pretty fine to me. Preemptively RTBC on the assumption that the tests will pass.

Added a separate issue to discuss the new interface #2857651: Introduce new interface for objects accepting constraints

The testbot doth protest too much.

@seanB, the stuff for {@inheritdoc} has been a confusion for pretty much as long as we've used it. https://www.drupal.org/node/1354#inheritdoc says:

This must be the only line in the docblock.

Also see #1994890: Allow {@inheritdoc} and additional documentation.

1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -530,6 +530,10 @@ public function getConfig($bundle) {
   +    // Note that constraints added via this method are not stored
   +    // in configuration and as such need to be added at runtime using
   +    // hook_entity_bundle_field_info_alter().
   

   Also wrapping too soon.

2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -186,6 +186,27 @@ public function getSetting($setting_name);
   +   * NOTE: This will overwrite any previously set constraints.
   +   * In most cases DataDefinitionInterface::addConstraint() should be
   +   * used instead. See DataDefinitionInterface::getConstraints() for
   +   * details on how constraints are defined.
   

   This is wrapping too soon I think.

Method additions are allowed in minors per https://www.drupal.org/core/d8-bc-policy:

Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:
Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.
Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

That said, I'm not sure that we've addressed the last part of #16? That sounded like a more BC solution to me. I think that exploring that rather than the interface break is worthwhile before we add the BC break. (Even if we do decide to go ahead with the method addition.)

Finally, either way, it will need a change record. Thanks!

Here is a new patch implementing the new interface. Discussed this phenaproxima some time ago and he suggested ConstrainableDataDefinitionInterface().
Please let me know if this is what it should be, I will start writing the change records when there is consensus on the scope of this issue.

Hmmm I missed that ContextDefinitionInterface is in component and that DataDefinitionInterface is in core. I definitely think that that means discussing this part in #2857651: Introduce new interface for objects accepting constraints makes more sense than trying to do that here.

Regarding BC. I think given we have classes DataDefinition and FieldConfigBase - both which implement setConstraints we are okay here. There is no alternate implementation of TypedData or the field system - especially an alternate implementation based only the interfaces. This is almost certainly impossible.

Back to #20 and added the minor changes for #25.1 and #25.2.

Also added a change record: https://www.drupal.org/node/2864299

It would be nice to describe in the issue summary what the usecase is of setConstraints vs. just addConstrains. At least for me increasing an API surface is not necessarily the best idea.

So as far as I can tell, this is basically the typed-data equivalent of #2346329: hook_entity_base_field_info_alter() and hook_entity_bundle_field_info_alter() are documented to get a parameter that doesn't implement an interface with setter methods. So would be nice to take that into account, so that we don't end up pursuing different directions in the two issues.

I've updated the summary and added a example of usage. The biggest difference between setConstraints and addConstraint is you can overwrite existing constraints through setConstraints. It is not used a lot though.

I found this while working on the media module. We are adding constraints on field/entity level when I noticed setConstraints was missing on the DataDefinitionInterface.
$this->get($source_field_name)->getDataDefinition()->setConstraints($source_field_constraints);

While I agree with @dawehner, it increases our API surface. This is already implicitly part of the API surface since setConstraints is already implemented on both DataDefinition and FieldConfigBase.

About the patch:

Can we remove the setConstraints method from FieldConfigInterface? According to #25 it can't.

Wondering if this is something still desired?

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Should we deprecate the function out of FieldConfigInterface vs removing it?