Split from #1696648: [META] Untie content entity validation from form validation

Problem/Motivation

Validation in Drupal is tied to the Form API, we need a proper validation framework so that validation is defined by the objects/entities that needs to be validated, while still allowing the use of Constraints on FAPI.

This will allow validation when using:

  • Validation of web service calls
  • Node import, migrate
  • Edit (Spark)

Proposed resolution

As discussed in #1696648: [META] Untie content entity validation from form validation, we have the options of rolling our own vs. using the Symfony validator component. Thanks to the help of Bernhard Schussek (bschussek) from symfony, integrating the symfony component works well now. So the proposal is to build upon that to implement validation for typed data, what covers entity validation. As a follow-up, we could then allow using symfony validation constraints directly from fapi also.

User interface changes

None.

API changes

None for now. Typed data validation is something the API prepared for, but wasn't implemented yet. If we decide to use it as the only way to validate forms as a follow-up, that obviously would be a rather big API change.

Code

The code is developed in the entity api improvements sandbox, under the validation and validation-symfony (only symfony component) branch. The issue for adding the symfony component is #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3., upon which the patches posted here build.

Implementation

Summary

  • Validation constraints can be defined in type data defintions below the 'constraints' key. If the data type specified already 'constraints' at its plugin definitions, those are applied as well. This will be important for further added data types like 'email', which should validate the value is an email.
  • There is built-in validation support for all primitive types of the typed data API. The constraint is added automatically once the mapping to a primitive type is specifeid in the type plugin definition, e.g. if the 'email' type specifies it is a string, the PrimitiveType constraint will be added to check its a 'string' also.
  • There are some general Drupal-specific validation classes which override symfony-classes for making violation messages translatable. Then, there are some typed data specific validator-metadata implementations which are needed for validating typed data. For implementing the symfony validation API for forms I'd expect us to implement similar classes for forms also, but that can be investigated further in a form follow-up issue.

Patch notes

  • Typed data constraints shoud be moved below the TypedData component, what requires #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory to be fixed first.
  • The typed data manager should be handled as proper dependency in the typed data classes. I'd prefer to not do that in this patch though as it would just result in conflicts with #1778178: Convert comments to the new Entity Field API. Let's better handle it over there or in a separate follow-up.
  • The patch changes how the primitive typed data objects deal with invalid values. Instead of throwing exceptions when an invalid data type is set, the value is just set. Instead, validation will fail afterwards. This means, that you can only really rely on typed data definitions to apply if validation gives you a green light, but I think that's reasonable.
  • Validation constraint plugins are currently not namespaced. For core, that might make sense to do in order to provide a better DX. It's easy to avoid name clashes from inside core, however modules would have to properly prefix their constraint plugin ids.
  • Translation violation messages is figured out at #1853096: Integrate symfony validation violations with Drupal translation.

Todo

Follow-ups


Original report by attiks

Timing

Since there are only 10 days left before feature freeze, I would love to know what has to be committed before December 1st and what is allowed to be added/done after December 1st. I can make time to work on this, but only if I know this has a chance to be added to Drupal 8, otherwise I probably can spent my time on other issues.

Proposed resolution

As discussed in #1696648: [META] Untie content entity validation from form validation, we have to options:

  1. Use the Symfony validator component, but this is designed to work using metadata about a class, not with an instance, a widget or a form not tight to an entity. Bernhard Schussek (bschussek) is investigating how it can be changed so we can use it.
  2. Roll our own framework, a starting patch can be found in #1696648: [META] Untie content entity validation from form validation, it supports everything from the list below except 6 and 8

We need to be able to specify Constraints on

  1. TypedData definitions (ex: Drupal\Core\TypedData\Type\Integer can only containts NULL or an integer)
  2. Field definitions (ex: an integer field only accepts numbers)
  3. Entity definitions (ex: a node needs a title)
  4. Form API, for forms/form elements not tight to an entity (ex: captcha)
  5. Field instance (ex: a user reference can only reference certain roles)
  6. Field widgets (ex: a user reference field accepts a user name as input, but stores an integer)
  7. Constraints need to be translatable
  8. Allow easy transfer of the Constraints to the client side

Remaining tasks

  • Create Constraints (plugins) so they can be used as a class and using the drupal_container, a constraint defines a condition that has to be checked like required, integer, range, ... Some are already provided, but creating all of them will take some time.
  • Alter the definition of TypedData and Fields to use those Constraints, this is already done in the patch
  • Remove the validation of TypedData and Field, this removes the existing implementation of the validation and is basically cleanup
  • Alter core/modules/field_ui/field_ui.admin.inc so a user can select constraints on a field instance, the patch includes part of this, but it needs to be further extended with a form so people can enter some data needed for a constraint. Example: the min_integer constraint allows you to define a minimum value for an integer field, the user has to enter that value in the field instance settings form.
  • Remove the validation of Field Widgets, this is cleanup, it removes all existing settings related to validation
  • Change drupal_get_form so it supports Constraints on custom forms, the patch only addresses forms build using a form controller, this needs to be extended so we can support custom forms, not tight to an entity, as well.
  • Allow custom binding between FAPI and entity, for the moment the patch assume that the form api key is the name of the field, but this isn't always acceptable, so we should allow people to specify which form api element is bound to which field of an entity

User interface changes

Field admin UI will change, the interface for constraints/validation will be made more uniform and easier to extend.

API changes

Yes, but most of it is new. The goal is to remove all form_validation except for specific things that can not be done by a constraint.
For the moment the idea is to make the validation phase optional, it will not be enforced when calling save() on an entity, it's only enforced during form_validate (like it is now).

#required can be converted to a Constraint (but will not cease to exist)
#maxlength can be converted to a Constraint (but will not cease to exist)
Most of the #element_validate can be replaced by constraints
Most of the #validate
_form_validate can be simplified


CommentFileSizeAuthor
#131 drupal8.bundle-validation-message.1845546-131.patch928 bytesstefan.r
#125 d8_validation-125.patch80.04 KBeffulgentsia
#123 d8_validation.patch80.04 KBfago
#123 d8_validation.interdiff.txt4.16 KBfago
#113 d8_validation.patch79.79 KBfago
#113 d8_validation.interdiff.txt2.94 KBfago
#110 d8_validation.interdiff.txt968 bytesfago
#110 d8_validation.patch79.8 KBfago
#107 d8_validation.patch78.86 KBfago
#107 d8_validation.interdiff.txt8.23 KBfago
#105 interdiff-do-not-test.patch2.54 KBEclipseGc
#104 validate.patch77.13 KBEclipseGc
#102 d8_validation.interdiff.txt1.48 KBfago
#102 d8_validation.patch75.74 KBfago
#101 d8_validation.interdiff.txt9.79 KBfago
#101 d8_validation.patch75.74 KBfago
#100 d8_validation.patch75.77 KBfago
#100 d8_validation.interdiff.txt9.97 KBfago
#99 d8_validation.interdiff.txt2.61 KBfago
#99 d8_validation.patch69.86 KBfago
#93 validate.patch69.76 KBEclipseGc
#92 validate.patch69.57 KBEclipseGc
#90 d8_validation-both-patches.patch772.72 KBfago
#90 d8_validation.to-symfony.interdiff.txt69.57 KBfago
#87 d8_validation.to-symfony.interdiff.txt68.95 KBfago
#87 d8_validation-both-patches.patch772.11 KBfago
#83 validation.patch2.93 KBEclipseGc
#77 d8_validation.patch737.47 KBfago
#77 d8_validation.interdiff.txt1.9 KBfago
#77 d8_validation.to-symfony.interdiff.txt71.28 KBfago
#75 d8_validation.patch735.72 KBfago
#75 d8_validation.interdiff.txt69.53 KBfago
#62 d8_validation.interdiff.txt11.23 KBfago
#61 d8_validation.patch761.47 KBfago
#61 d8_validation.to-symfony.interdiff.txt69.93 KBfago
#42 d8_validation.interdiff.txt872 bytesfago
#42 d8_validation.patch766.3 KBfago
#42 d8_validation.to-symfony.interdiff.txt74.75 KBfago
#38 d8_validation.patch765.58 KBfago
#38 d8_validation.to-symfony.interdiff.txt74.04 KBfago
#30 d8_validation.interdiff.txt28.75 KBfago
#29 d8_validation.patch1.16 MBfago
#29 d8_validation.to-symfony.interdiff.txt71.33 KBfago
#19 d8_validation.patch1.15 MBfago
#19 d8_validation.to-symfony.interdiff.txt68.59 KBfago
#17 d8_validation.patch1.15 MBfago
#17 d8_validation.to-symfony.interdiff.txt67.68 KBfago
#15 d8_validation.to-symfony.interdiff.txt31.91 KBfago
#15 d8_validation.patch1.12 MBfago
#3 i1696648-79.patch60.62 KBattiks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

Title: Copy of Untie entity validation from form validation » Add a validation framework to Drupal 8
Priority: Normal » Major

First thing we need to figure out is what needs to be comitted before feature freeze, and what can be done after?

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Issue summary: View changes

Updated issue summary.

pounard’s picture

Getting SF validation API/Component first would be all of the first, biggest and easiest step to begin with.

attiks’s picture

FileSize
60.62 KB

@pounard the problem is that symfony has to be changed before it can be used for Drupal. Using SF would be great, but will have to now if there Validator will allow everything we need.

Attached the latest patch from #1696648: [META] Untie content entity validation from form validation that allows defining constraints on classes and instances.

fago’s picture

My 2 cents:

TypedData definitions (ex: Drupal\Core\TypedData\Type\Integer can only containts NULL or an integer)
Field definitions (ex: an integer field only accepts numbers)
Entity definitions (ex: a node needs a title)
Form API, for forms/form elements not tight to an entity (ex: captcha)
Field instance (ex: a user reference can only reference certain roles)
Field widgets (ex: a user reference field accepts a user name as input, but stores an integer)
Constraints need to be translatable
Allow easy transfer of the Constraints to the client side

With having 'constraints' in typedData definitions (as planned), we'd automatically solve the points field+entity definitions - not? Field definitions are typed data definitions, combinated entity-level constraints we'd have to place somewhere, but setting the 'title' to required should be fine by putting the constraint on the title field.
Then, yep making it possible to re-use the validation API from FAPI makes a lot of sense, but shouldn't be hard. Once we solved form API, we also solved field widgets. Constraints on the client side would be a follow-up issue for me.

attiks’s picture

With having 'constraints' in typedData definitions (as planned), we'd automatically solve the points field+entity definitions - not?

It depends if we're going to create a TypedData for each possible field, for example a dimension field contains 3 integers (typedData), but can has a constraint on the field level (min 1m³) that cannot be defined on the TypedData level.

The same goes for custom entities defined in a module (so not node, user, ...), the developer probably wants to add constraints in the definition of the entity.

The general idea is to give developers enough options so they can handle the constraints how they see best fit, since we have 3 levels (entity, field, typedData) it makes sense to allow them to define constraints on all 3 levels.

fago’s picture

It depends if we're going to create a TypedData for each possible field, for example a dimension field contains 3 integers (typedData), but can has a constraint on the field level (min 1m³) that cannot be defined on the TypedData level.

We have typed data definitions for the field level also? See fieldDefinitions() in the storage controller.

fago’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

Status: Active » Needs review

Switching to needs review for "Since there are only 10 days left before feature freeze, I would love to know what has to be committed before December 1st and what is allowed to be added/done after December 1st. I can make time to work on this, but only if I know this has a chance to be added to Drupal 8, otherwise I probably can spent my time on other issues."

I would love to see this happen because it will solve a lot of existing troubles and it will facilitate the new things for Drupal 8 like inline editing, custom error messages in forms, json, ...

webchick’s picture

I have no earthly idea how to answer that question. Can the issue summary be updated for benefit of people not deeply involved in this issue to know what details like "Create Constraints (plugins) so they can be used as a class and using the drupal_container" means?

webchick’s picture

But in general, this is a new major API, so would be bound to feature freeze.

webchick’s picture

Issue summary: View changes

Updated issue summary.

attiks’s picture

I've updated the summary, to summarize, most of the things are already in the patch, the idea is to propose a working patch and delay the writing of constraints till after feature freeze as well as the removal of the existing validation logic. But I have to know it that's an acceptable approach before I start.

effulgentsia’s picture

At the very least, I think #1696648-83: [META] Untie content entity validation from form validation is a critical task not bound to feature freeze. I'm not that clear on the relationship between this issue and that one. Possibly it makes sense to make this one critical instead. I don't think we should make both of them critical though, since there's so much overlap that it would unfairly affect thresholds.

effulgentsia’s picture

If #10 is suggesting that we get the basic framework in place before feature freeze in this issue, and then implement the details needed by #1696648: [META] Untie content entity validation from form validation in that issue after feature freeze, that seems like a good approach to me.

webchick’s picture

If the proposal is "get a big API change in (including a couple of decent reference implementations) before feature freeze, but perform full-scale conversion of everything to use it post-feature freeze" (which attiks confirmed it is) then that sounds consistent with other major features (Twig, CMI, WSCCI, etc.) so I think that's an acceptable plan of attack.

fago’s picture

Assigned: attiks » fago

Update: bschussek has created an initial PR for the changes we need for being able to use the symfony validator, see https://github.com/symfony/symfony/pull/6096. I'll try to use this and merge it with what we have (#3).

For now I just put together some classes to verify the changes work for use, see http://drupalcode.org/sandbox/fago/1497344.git/tree/refs/heads/validatio... I'll continue to work on an acutally working implementation in my entity-api improvements sandbox under the validation branch tomorrow. Thus taking over for now.

fago’s picture

As said I've worked on this today and got it basically working. Patch is attached. Short summary:

- Validation constraints are specified in the typed data definition below the 'constraints' key.
- Validation constraints are registered via our plugin system, what gives us metadata to build UIs and discovery to allow devs to find them. Symfony constraints are added in via a discoverydecorator.
- Constraint objects can be retrieved for a object by calling $typed_data->getConstraints().
- Finally, the typeddata API implements validator metadata interfaces to let the validator work with typed data.

Todo:
- Translate violation messages
- Make sure validation structures works properly and fix the NULL constraint for them (see test fail).
- Make sure groups are supported by our metadata classes (could be a follow-up)
- Implement validadtion for all basic typed data types
- Define how 'types' are specified for validation plugins.

Symfony validator changes got commited, thus I've created #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3.. By commiting the component separately we can keep this patch small and reviewable.

Patch is attached and should have 1 fail. One containing everything and one interdiff against #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3..

Status: Needs review » Needs work

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
67.68 KB
1.15 MB

Updated the patch. This is already quite complete.

Summary

  • Validation constraints can be defined in type data defintions below the 'constraints' key. If the data type specified already 'constraints' at its plugin definitions, those are applied as well. This will be important for further added data types like 'email', which should validate the value is an email.
  • There is built-in validation support for all primitive types of the typed data API. The constraint is added automatically once the mapping to a primitive type is specifeid in the type plugin definition, e.g. if the 'email' type specifies it is a string, the PrimitiveType constraint will be added to check its a 'string' also.
  • There are some general Drupal-specific validation classes which override symfony-classes for making violation messages translatable. Then, there are some typed data specific validator-metadata implementations which are needed for validating typed data. For implementing the symfony validation API for forms I'd expect us to implement similar classes for forms also, but that can be investigated further in a form follow-up issue.

Patch notes

  • Typed data constraints shoud be moved below the TypedData component, what requires #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory to be fixed first.
  • The typed data manager should be handled as proper dependency in the typed data classes. I'd prefer to not do that in this patch though as it would just result in conflicts with #1778178: Convert comments to the new Entity Field API. Let's better handle it over there or in a separate follow-up.
  • The patch changes how the primitive typed data objects deal with invalid values. Instead of throwing exceptions when an invalid data type is set, the value is just set. Instead, validation will fail afterwards. This means, that you can only really rely on typed data definitions to apply if validation gives you a green light, but I think that's reasonable.
  • Validation constraint plugins are currently not namespaced. For core, that might make sense to do in order to provide a better DX. It's easy to avoid name clashes from inside core, however modules would have to properly prefix their constraint plugin ids.
  • Translation works, but we'll have to modify our translation extraction routine to get the messages to translate. This can be done by looking up all properties of constraint plugins that have the 'message' suffix. Not ideal, but that should be doable.

Todo:

  • Implement the Bundle constraint.
  • Define/document how constraints can specify which types they support. Rename ConstraintManager::getList() to getDefinitionsByType() and test it.
  • Automatically add in the NotNull constraint if 'required' is set and test that works.
  • The patch currently rellies on some small modifications to the symfony validator component: https://github.com/fago/symfony/commit/b793ed3172f8dd126d219eea36afa6507... We need to figure out whether this could go into upstream.

Follow-ups:

  • Add in plugin definitions for all Symfony validation constraints that are useful for us.
  • Use it to implement #1696648: [META] Untie content entity validation from form validation
  • Make sure validation groups are supported by our metadata classes
  • Implement validation of the uri type to accept all URIs as specified by the RFC.
  • Allow using symfony validation constraints for form validation
  • Allow configuring validation constraints via field UI

Assigned: fago » Unassigned

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

Fixed the test fail.

attiks’s picture

Nice work, I had a quick dreditor look and this looks amazing, I'll have a closer look later today.

yched’s picture

I only skimmed through this for now, just a couple remarks :

+        'constraints' => array(
+          'Min' => array('limit' => 0),
+        ),

Keying by constraint class name will prevent using the same constraint twice in the same constraint set - wouldn't there be use cases for that ?

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/EntityType.php
...
+class EntityType extends Constraint {

Having a constraint class named EntityType sounds confusing. According to our code standards : "Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace". On that regard, just "EntityType" is highly confusing :-)
Possibly applies to the other constraint classes as well, but EntityType is the one that striked me most.

+    $container->register('typed_data', 'Drupal\Core\TypedData\TypedDataManager')
+      ->addMethodCall('setValidationConstraintManager', array(new Reference('validation.constraint', ContainerInterface::IGNORE_ON_INVALID_REFERENCE)));

Why do we need a setValidationConstraintManager() method ? couldn't the constraintManager be injected in TypedDataManager::__construct() ?
Similarly for getValidator() / setValidator(), could new Validator(new MetadataFactory(), new ConstraintValidatorFactory()) be injected from the DIC ?

Also, not for this patch to fix, but to comply with the rest of core, the typed_data() wrapper should go away in favor of direct drupal_container()->get() reads, and the DIC entry should be something like 'plugin.manager.entity.typed_data' instead of 'typed_data'.
Reading at the code here, it's pretty non-obvious that typed_data() is the plugin manager.

fago’s picture

Keying by constraint class name will prevent using the same constraint twice in the same constraint set - wouldn't there be use cases for that ?

Good question. I've not been able to come up with a use case for it yet - do you have an example?
We obviously could so somethink like
'constraints' => array(
array('Min' => array('limit')),
),
but I think the other one is nicer + makes it easy to find/read constraint option if constraints are specified via UI. We could support both structures at the same time by limit plugins to start with a character, i.e. if the key is numeric treat it as above?

Having a constraint class named EntityType sounds confusing. According to our code standards : "Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace". On that regard, just "EntityType" is highly confusing :-)

True, however in this case you won't ever instantiate or type-hint the constraint classes itself. Symfony uses simple class names like "NotNull" also so I've been following that. However, we could go and add a Constraint suffix (EntityTypeConstraint) if that works better for us?

Why do we need a setValidationConstraintManager() method ? couldn't the constraintManager be injected in TypedDataManager::__construct() ?
Similarly for getValidator() / setValidator(), could new Validator(new MetadataFactory(), new ConstraintValidatorFactory()) be injected from the DIC ?

The idea behind that is that the dependencies are optional, i.e. if no validation is needed the classes won't be loaded/instantiated. The typed data manager will be need for loading entities also, but validation not - so I think it's a good idea to keep it optional.

Also, not for this patch to fix, but to comply with the rest of core, the typed_data() wrapper should go away in favor of direct drupal_container()->get() reads, and the DIC entry should be something like 'plugin.manager.entity.typed_data' instead of 'typed_data'.
Reading at the code here, it's pretty non-obvious that typed_data() is the plugin manager.

Yes, as already noted in the patch notes. Not sure whether we already have guidelines for DIC structure, but we should follow whatever we have - yep.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

I updated the issue summary based upon #17.

yched’s picture

re: Keying by constraint class name
I think supporting both formats (keyed by number / keyed by class names) would make painful code, and inconsistent constraints definitions.
I'm also not sure about actual use cases for reusing the same constraint twice, but I don't have a full vision on the available set of constraint types.
I'd say a regexp validator (dunno if Symfony provides one, but if not this sounds like an addition that will most probably happen in contrib if core doesn't provide one) could definitely be used multiple times...
At any rate, not supporting reuse sounds like painting ourselves in a corner, since we cannot really tell what kind of constraints contrib will provide.

+1 on adding a "Constraint" suffix on our own constraint classes (EntityTypeConstraint rather than EntityType). Symfony has its own code standards, but right now EntityType violates ours.

The idea behind that is that the dependencies are optional, i.e. if no validation is needed the classes won't be loaded/instantiated. The typed data manager will be need for loading entities also, but validation not - so I think it's a good idea to keep it optional

Well, the constraintManager doesn't seem optional right now, since CoreBundle::build() does a addMethodCall('setValidationConstraintManager') on the typed_data entry ?
I'm just saying that, unless we have a case for swapping validation constraint manager at runtime (do we ?), then providing no setValidationConstraintManager() method, and injecting the manager in the constructor instead sounds clearer.

fago’s picture

Well, the constraintManager doesn't seem optional right now, since CoreBundle::build() does a addMethodCall('setValidationConstraintManager') on the typed_data entry ?

True, but it should be optional also. I'll fix it that way.

I'd say a regexp validator (dunno if Symfony provides one, but if not this sounds like an addition that will most probably happen in contrib if core doesn't provide one) could definitely be used multiple times...

Symfony has one, yeah. Not sure this is a good use-case, but I agree we have to account for this anyway. So what about the following: Have

array(
  'Min' => array('limit' => 5)
),

by default, but allow overriding it via the 'constraint' key:

array(
  'Min' => array('limit' => 5),
  'Min2' => array('constraint' => 'Min', 'value' => array('limit' => 5)),
),

That makes one fixed structure while not putting us in a corner. Symfony has already support for handling options with that 'value' key built in, so we'd just extend that with supporting the 'constraint' key.

tim.plunkett’s picture

I've not been able to come up with a use case for it yet

This reminds me of #states, in which the API had to be expanded later to fit the use cases of contrib.
If you ask that question, someone will find a use case :)

yched’s picture

re #25 / syntax :
Yes, I guess that would work. Generally speaking, I'm personally not a fan of alternate / dual syntaxes, they tend to get more confusing than helpful in the end. A numerically index array of constraints with a 'constraint' property holding the constraint class name (or plugin id ? aren't constraints plugins ?) would just work (tm) IMO.

Also, not sure if validation breaks at the 1st failed constraint, or if all constraints are validated, but if the former, order would be important, so numerically indexed arrays of constraints would make sense.

attiks’s picture

#25, #26 we need a way to add the same constraints twice, I prefer the second explicit syntax

#27 all constraints are validated, in the end you get an array of violations, SF allows you to define groups of constraints (on one property) which can be used conditionally, if the first group passes, the second group is validated. Ex. Entity reference, first group checks required and is_numeric, second group does a database lookup to see if the entity exists.

fago’s picture

I think commonly the extra array-layer is unnecessary, so I think supporting the 'short' notation makes sense. So I've implemented it as suggested in #25, please check attached patches.

While doing performance checks in the entity-NG comment conversion issue I also noted that another array-level requires quite some more memory if repeated often (what we'll have here), so reducing array layers by default would probably make sense memory consumption wise also. At the issue over there it were 2MB per array level (key "0") with 4000 array instances.

Attached patch also fixes constraint class names and the optional dependency to be really optional, but still does setter injection. Is optional constructor injection preferred? I'm not sure what's better or what is the usual way of doing it?

fago’s picture

FileSize
28.75 KB

Attaching an interdiff to #19.

yched’s picture

Attached patch also fixes constraint class names and the optional dependency to be really optional, but still does setter injection. Is optional constructor injection preferred? I'm not sure what's better or what is the usual way of doing it?

I'd tend to consider that setter injection is preferred when being able to injected different stuff on the same object at runtime is a needed feature. Otherwise, constructor injection works, avoids cluttering the interface with a setter, and makes the overall mechanics much clearer when looking though code.
That's only my 2cts though :-)

attiks’s picture

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/EntityTypeConstraint.phpundefined
@@ -0,0 +1,52 @@
+  public $message = 'The entity must be of type {{ type }}.';

Quick question: why don't we use Drupal placeholders in here?

fago’s picture

We could - this will default %type placeholders anyway. I've been following symfony here so it would be consistent across all constraints. Alternatively, we can override all symfony default messages having placeholders with messages using drupal placeholders also?

attiks’s picture

Since we wrap them already it probably is the best to use drupal placeholders, so it's consistent.

fago’s picture

+1 on adding a "Constraint" suffix on our own constraint classes (EntityTypeConstraint rather than EntityType). Symfony has its own code standards, but right now EntityType violates ours.

While I've already done so, I wonder whether we should rever this commit. As bschussek pointed out, this makes any constraints not conform with the upstream pattern, what would make contributing them back harder.

Actually, it's the same question we have with replacement patterns: The usual symfony pattern of { replacement } would work for us also, thus adopting it would make it easier to contribute any Drupal constraints back also.

Opinions on that?

attiks’s picture

In practice, how many Constraints do you see to go up stream? AFAIK SF has most common already implemented.

I think we have to consider the Drupal coding standards, so i prefer wrapping the SF Constraints inside a Drupal variant and changing the message and name to follow are coding standards.

fago’s picture

In practice, how many Constraints do you see to go up stream? AFAIK SF has most common already implemented.

I agree that most constraints are probably not of interest for upstream. I see us adding mostly adding constraints implementing Drupal-specific stuff. Still, being able to contribute them back as easy as possible is probably a good thing.

fago’s picture

Attached patch implements the following todo items:

  • Implement the Bundle constraint.
  • Define/document how constraints can specify which types they support. Rename ConstraintManager::getList() to getDefinitionsByType() and test it.
  • Automatically add in the NotNull constraint if 'required' is set and test that works

Also, I've found a better way to deal with empty data structures + symfony constraints. I changed our appraoch to pass on NULL values to the validator if the typed data object is empty - that makes it work with existing null,notnull,blank,notblank constraints out of the box. Thus, we can re-use them without extending them now as well.

So what's left is how to solve the upstream PR issue by finding the best way to provide our custom Violation class for translation. (Right now the patch adds in the not-commit symfony PR). bschussek offered to have a look at it already.

Lars Toomre’s picture

@fago - I realize that this is a work in progress issue. However looking at the interdiff from #38, I noticed a couple of things:

a) My understanding is that there needs to be a blank line betwee '<?php' and the start of the @file docblock.

b) My understanding also is that blank lines are needed after a class opening brace and before the class declaration closing brace. There appear to be many blank lines missing before the closing brace of class declarations.

Status: Needs review » Needs work

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

I discussed the open point with bschussek via Skype. We came to the conclusion that the cleanest and best decoupled solution is using a separator translator for handling violation message translations. Naturally, that would be the already existing and rather simple TranslatorInterface of symfony. Problem being that this is part of the Translation component, which we do not use.

Question is how we solve that now? Should we add the whole component just for that? Or just add the interface? I'm opening another issue to discuss that: #1852106: Add the symfony translator interface or translation component

Note: We only need the interface, we'll provide our own implementation anyway.

fago’s picture

Status: Needs work » Needs review
FileSize
74.75 KB
766.3 KB
872 bytes

a) My understanding is that there needs to be a blank line betwee '<?php' and the start of the @file docblock.

My understanding was the contrary. Core seems to be both? I've not been able to find a coding style guideline here? So if there is none I'd suggest taking care of that in a separate issue.

b) My understanding also is that blank lines are needed after a class opening brace and before the class declaration closing brace. There appear to be many blank lines missing before the closing brace of class declarations.

Again my understanding is different here. As I already stated over at #1839078-10: Implement the new entity field API for the email field type I'm unable to find this in the coding standards except for an example doing it without the new line. Imo clarifying this should be a separate issue also

Attached patch fixes the test fail - the EntityNG term reference field went in :)

xjm’s picture

While I believe @Lars Toomre is correct, both our codebase and our standards documentations are inconsistent about those blank lines.

@Lars Toomre, I'd suggest to forego pointing out whitespace errors and such with an 800K patch. Also, as we discussed, correction of these sorts of fine points of the coding standards is best given as a rerolled patch with an interdiff, especially at this point in the development process. Thanks!

Lars Toomre’s picture

Regarding #38 and #42 a), issue #1852152: [policy, no patch] Blank line before @file docblock in PHP file types? has been opened as a follow up.

webmozart’s picture

Keying by constraint class name will prevent using the same constraint twice in the same constraint set - wouldn't there be use cases for that ?

We have the same "problem" in Symfony's annotation and YAML driver for the Validator. This means that, once we or you stumble across an issue with that, we need to solve it in a general fashion anyway. So far, this wasn't a problem.

+1 on adding a "Constraint" suffix on our own constraint classes (EntityTypeConstraint rather than EntityType). Symfony has its own code standards, but right now EntityType violates ours.

I would strongly opt against that. For Symfony, constraint names in the mapping always refer to class names, hence the simple class names. The namespacing problem is solved similar as in XSD (example from the YAML driver here):

namespaces:
  custom: Vendor\Namespace\Constraints\

Vendor\Namespace\Model\Entity:
  constraints:
    # Constraint in the default namespace (configured in the mapping loader)
    - NotNull: ~
    # FQN
    - Symfony\Component\Validator\Tests\Fixtures\ConstraintA: ~
    # Namespace prefix configured within the mapping file
    - "custom:ConstraintB": ~

(I generally recommend you to check the YAML driver examples, which have a very similar syntax to yours - just in YAML instead of PHP arrays).

It is unlikely that any of Drupal's core constraints will ever be used in a Symfony-only context, but the ones created by the Drupal community sure will, and the community will follow your example (i.e. "Constraint" suffix). This will create a divide between the constraints created by the Symfony and the Drupal community, which I'm sure we both don't want.

Quick question: why don't we use Drupal placeholders in here?

For the same reason, I recommend you to use the "{{ var }}" syntax in the constraint messages defined in PHP code. Your implementation of TranslatorInterface needs to convert the parameters from "{{ var }}" to "%var" anyway in order to support constraints by the Symfony community. Your translation files can then use the "%var" syntax.

yched’s picture

(regarding reuse of the same constraint in a constraint set) We have the same "problem" in Symfony's annotation and YAML driver for the Validator. This means that, once we or you stumble across an issue with that, we need to solve it in a general fashion anyway

Thing is, in some cases the constraint set is going to be editable through a UI. So if the underlying format for constraint sets doesn't allow reusing the same constraint twice, we'll need to bake that limitation in the UI beforehands.
Thus I'd find it more reasonable to go for a format that allows reuse straight ahead ?

For Symfony, constraint names in the mapping always refer to class names, hence the simple class names

I'm not sure this applies here. Since in the drupal use, Constraint classes are plugins (i.e drupal Plugin API), with a plugin_id that is a lowercase, underscore-separated machine name. Those are the strings that will be stored in config.

Status: Needs review » Needs work

The last submitted patch, d8_validation.patch, failed testing.

webmozart’s picture

I created a pull request that integrates TranslatorInterface into the Validator now. Once this is merged, you can implement your own translator and implement your translation strategy there.

There you will probably have to

  1. substitute all occurrences of "{{ var }}" in the message ID (=the message template) by "%var"
  2. change every message parameter (=key in the parameters array) from "{{ var }}" to "%var"

An alternative to 1. is to use our syntax "{{ var }}" in the "original version" of your translations and your syntax "%var" in the "translated version".

webmozart’s picture

I also recommend to use

$validator = Validation::createValidatorBuilder()
    ->...
    ->getValidator();

for constructing the validator. The constructor of the Validator class is not part of our public API, but Validation is.

attiks’s picture

#46 I must say I agree, it is nice to follow SF as close as possible, but we're probably going to extend the Constraint classes anyway, so we have to override the SF ones to add support for FAPI and client side integration, although the latter one (theoretically) can be done upstream as well.

To solve the BC with SF, we can instruct developers to write 2 classes for their constraints, one following the SF standards and API, and one for use inside Drupal. This will solve the translation problem as well, although I'm not opposed by adding this, so we can avoid some code duplication. Example: We can re-use the validate of NotBlankValidator, but we encapsulate it inside NotBlankValidatorConstraint (or RequiredConstraint to follow our own terminology)

fago’s picture

There you will probably have to

substitute all occurrences of "{{ var }}" in the message ID (=the message template) by "%var"
change every message parameter (=key in the parameters array) from "{{ var }}" to "%var"

Atually, we can directly go with the { var } notation also, that works with our translation system / t().

#46 I must say I agree, it is nice to follow SF as close as possible, but we're probably going to extend the Constraint classes anyway, so we have to override the SF ones to add support for FAPI and client side integration, although the latter one (theoretically) can be done upstream as well.

I discussed FAPI integration a bit with bschussek. It turns out there are multiple ways to do it without having to extend Constraints or ConstraintValidators. We could do either a custom "Form" constraint that takes over validating the whole form manually, or just do the same manually. Then we could pass on $form_state to the validator with an according form-metadata factory that extracts validation constraints out of $form - what appears to be the cleanest solution to me. Anyway, we can discuss that further in the form follow-up.
For client-side validation I'm not sure, but yes finding a solution inline with upstream would surely be desireable.

Summing up, I do not see the necessity for overruling symfony constraints or constraint validators, what is good is we want to use it - not overwrite it ;) But using existing violations means that we have symfony validators generating violation messages using the { var } pattern. So we could
a) transform that to our pattern in our translator + overwrite all messages. That would add some runtime overhead for translating patterns though.
b) take over symfony patterns for given constraints. What means Drupal translators would already have to deal with { value } translation patterns. For consistency across violation messages I'd vote for doing the same for Drupal violation messages then.

Alltogether, I see no strong reasons for violating upstream guidelines for class naming and replacement patterns - both just work for us, it's just not 100% up with our guidelines. So, for consistency with upstream and easing collaboration with upstream I think we should adopt both.

I would strongly opt against that. For Symfony, constraint names in the mapping always refer to class names, hence the simple class names. The namespacing problem is solved similar as in XSD (example from the YAML driver here):

We figured symfony YAML constraints actually do not have the problem, only annotations do. But, we also figured out a very simple way to overcome the limitation: Add a new constraint, "ListConstraint" or so, which allows adding in multiple constraints of the same name below. (There are already other symfony constraints what allow you to specify further constraints, e.g. to validate multiple nested fields).

Given that we can do that via an added constraint, I think we should drop the second way of specifying the array in favour of one simple way to do it.

Thing is, in some cases the constraint set is going to be editable through a UI. So if the underlying format for constraint sets doesn't allow reusing the same constraint twice, we'll need to bake that limitation in the UI beforehands.

Yes even though we do, I think that's something reasonable to do anyway. It should cover the 99% use cases and makes it easier to write and build the UI.

as #49: thanks, I'll do.

@Laars Toomre: Thanks!

fago’s picture

Title: Add a validation framework to Drupal 8 » Implement validation for the TypedData API

Re-titling the issue: This really is just about implementing validation for typed data, while #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3. adds the general validation API and http://drupal.org/node/1696648 the integration with entity forms.

Follow-up on translation: As the modifications are under work upstream I propose to go without translation in a first step. It's easy to take an violation message and translate it afterwards as we can access the message template and replacement parameters - so we can do that for now, until the issue is resolved upstream and found a solution for #1852106: Add the symfony translator interface or translation component.

Thoughts?

attiks’s picture

#51: "I discussed FAPI integration a bit with bschussek. It turns out there are multiple ways to do it without having to extend Constraints or ConstraintValidators". The idea is that if there's a Constraint it will automatically be added to the Form API. Example NotBlankValidator on a field will add '#required' => TRUE to the corresponding FAPI element, developers still can use form_alter/after_build to override it. The easiest way to do this is to extend the Constraint and add the code for FAPI as part of it.

fago’s picture

ad #53: I think validation should stay separated from forms, i.e. make fapi integrate validation - not the other way round. Thus, I'd possibly generate constraints based upon other fapi keys. Actually, that's exactly what the latest patch does for type data definitions also.

attiks’s picture

IRC discussion with fago, added for reference

[Wednesday 15:10]	fago	attiks, ping
[Wednesday 15:12]	attiks	fago: hi
[Wednesday 15:13]	fago	attiks, hi!
[Wednesday 15:13]	fago	attiks, I actually though to integrate fapi and validation the other way round, have fap integrate and call validation.
[Wednesday 15:13]	fago	attiks, just commented http://drupal.org/node/1845546#comment-6785556
[Wednesday 15:13]	Druplicon	http://drupal.org/node/1845546 => Implement validation for the TypedData API => Drupal core, entity system, major, needs work, 54 comments, 5 IRC mentions
[Wednesday 15:13]	attiks	reading
[Wednesday 15:14]	attiks	fago: what I wrote is just to mark a field #required (adding the red star in the ouput), if a Constraint makes the field required
[Wednesday 15:15]	attiks	fago: the actual validation is only after the submit
[Wednesday 15:15]	fago	attiks, I see - so it's about synchronizing things.
[Wednesday 15:15]	attiks	fago: correct
[Wednesday 15:15]	fago	attiks, still, I think we should keep that out of validation, i.e. fapi could take care of that
[Wednesday 15:16]	fago	attiks, so we have the systems properly untangled
[Wednesday 15:16]	attiks	fago: so we're do you want to store which Constraint alters which properties from a certain FAPI element
[Wednesday 15:17]	fago	attiks, if we go back to the simple array format for constraints only, that would be simple. just check $element['#constraints']['NotBlank']
[Wednesday 15:17]	fago	attiks, I'd say they are not supposed to alter fapi elements?
[Wednesday 15:17]	attiks	fago: and if I define a custom constraint, how is core going to know what to do?
[Wednesday 15:18]	attiks	fago: if we store everything inside the constraint class, it will be easy to extend
[Wednesday 15:18]	attiks	fago: BTW same goes for client side/javascript integration
[Wednesday 15:18]	attiks	fago: we use the Constraint class for the metadata, where ever it can be used
[Wednesday 15:18]	fago	attiks, If all we need is #required, we can hard-code support for required validation via NotBlank and NotNull and that's it.
[Wednesday 15:19]	attiks	fago: the ConstraintValidator use the Constraint to validate
[Wednesday 15:19]	fago	attiks, for client side I'm not sure, how do you see that working?
[Wednesday 15:19]	fago	attiks, yeah, the constraint should be just configuration.
[Wednesday 15:19]	attiks	fago: it isn't only about required, also about length, min/max, type, ....
[Wednesday 15:20]	attiks	fago: for clientside, the formcontroller adds constraints to the form/form element, the clientside validation calls constraint->getjs() to add the rules to the clientside
[Wednesday 15:21]	fago	attiks, I'd see us having default constraints in hook_element_info() as we have default #element_validate there. So we cover type. Then, what else applies to a certain input element is up to the input element, i.e. textfield could also support its maxlength key and sync it with a constraint. Just as it manually validates it now.
[Wednesday 15:21]	attiks	fago: it also calls constraint->getCustomRules so a constraint defined in contrib can define new js rules as well
[Wednesday 15:22]	fago	attiks, where is the difference between a custom rule and another one?
[Wednesday 15:22]	attiks	fago: type i mean the attribute from html like email, number
[Wednesday 15:22]	attiks	fago: jquery.validat.js contains some standard rules (like SF has some standard constraints), but they don't cover everything
[Wednesday 15:23]	fago	attiks, actualy, client side validation will depend on the widget, not? what makes it more tricky
[Wednesday 15:23]	fago	attiks, so the widget would have to implement some js-glue to normalize the data values on the client?
[Wednesday 15:23]	attiks	fago: if the widget does validation, yes
[Wednesday 15:24]	fago	attiks, no I mean because accessing the data values differs by widget
[Wednesday 15:24]	attiks	fago: ex entity reference: field expects an integer, widget allows text, so has to override the constraint to indicate it to the clientside
[Wednesday 15:25]	fago	attiks, It would be nice to have an abstract client side API to read/write options + default values for widgets. so you could also implement ajax-option updates very generically
[Wednesday 15:25]	attiks	fago: regarding custom rules, see http://drupalcode.org/project/clientside_validation.git/blob/refs/heads/7.x-1.x:/clientside_validation.js#l793
[Wednesday 15:25]	attiks	fago: "implement ajax-option updates very generically" would be nice indeed
[Wednesday 15:26]	fago	attiks, but beside that the widget may decide to use totally different input elements, or even multiple - consider a date with 3 selects vs. one textfield. We need to abstract that
[Wednesday 15:27]	attiks	fago: 1/ date as text field: field has DateConstraint, widget passes this to the clientside
[Wednesday 15:28]	attiks	fago: 2/ date as selects: field has same DateConstraint, widget adds own constraints to each select and to the group of the 3 selects
[Wednesday 15:29]	attiks	fago: off-topic, we cannot use SF DateConstraint since is only supports yyyy-mm-dd
[Wednesday 15:30]	fago	attiks, but how would my JS-rule would be able to check the over date value? it would have to be combined of all three selects.
[Wednesday 15:30]	fago	attiks, or maybe I have a input using timestamps. I'd have to convert that. But that should not be up to the constraint js-rule
[Wednesday 15:30]	attiks	fago: yes, but if the date is required, each select is marked as such.
[Wednesday 15:31]	attiks	fago: that is what we're doing in D6 and D7
[Wednesday 15:31]	fago	attiks, for required validation yes. what if I want to check that the date is <01.01.2012 ?
[Wednesday 15:32]	fago	attiks, to avoid requiring each constraint to support all widgets, the widget would have provide some JS to get the value?
[Wednesday 15:32]	attiks	fago: MaxDateConstraint(...) with a js_rules defined in the constraint 'maxdate'
[Wednesday 15:33]	attiks	fago: the glue is provided by code that reads all rules from all elements and ties it together
[Wednesday 15:33]	fago	attiks, I'm unsure that works well, but anyway
[Wednesday 15:33]	fago	attiks, you'll figure it out
[Wednesday 15:34]	fago	attiks, But yes, we'd need to possibility to add custom JS somewhere. not sure whether it fits to the constraint or the constraint validator. Maybe you could discuss that with bschussek? I mean if you could find a general solution that would be superb as potentially we coudl get js-rule from upstream then also.
[Wednesday 15:34]	attiks	fago: see http://drupalcode.org/project/clientside_validation.git/blob/refs/heads/7.x-1.x:/clientside_validation.module#l288 it gets all the rules and outputs them as settings
[Wednesday 15:34]	attiks	fago: settings are read on the clientside by jquery.validate
[Wednesday 15:34]	attiks	fago: if this can go upstream, great
[Wednesday 15:35]	fago	attiks, looks like you got it already
[Wednesday 15:35]	fago	attiks, yeah, best try to reach him on skype. it's easier to discuss that way
[Wednesday 15:36]	attiks	fago: i'm going to add the irc log to the issues, ok?
[Wednesday 15:36]	fago	attiks, so I'll drop the second array format, k? If we need constraints configured twice we can do the listconstraint.?
[Wednesday 15:37]	fago	attiks, k
[Wednesday 15:37]	attiks	fago: so listconstraint will allow same constraint twice, if so, ok by me
[Wednesday 15:38]	fago	attiks, yeah. that way we'd have only one and simple way to specify constraints
yched’s picture

FWIW, I think the constraint classes provided by symfony will need to be extended to be used on the drupal side anyway, since we'll need methods to support admin UIs (like Min::settingsForm() : outputs a textfield to let you enter the min value).
So, we can totally go for MinConstraint extends \Symfony\...Validation\...\Min while we're at it.

[edit: several words missing :-/]

attiks’s picture

#56 add the settings form to the constraint will make it similar to fields, so maybe that's a good idea. For now @fago added methods to get a list of parameters (all and/or optional ones), I think he wanted to add a generic form for all constraints. @fago correct me if I'm wrong.

fago’s picture

Actually, I've not taken care of an UI for configuring constraints in any way yet, but allowing the constraint plugin to provide a configuration form would work. (api first.. ;-) I don't think we want or need all constraints exposed to the UI though. What we could do also, is using parameter definitions for constraint options and generate forms on that - as I'd go for with actions: #1846172: Replace the actions API.

attiks’s picture

#52 Translation can be added in a follow-up

fago’s picture

Issue tags: -form API
fago’s picture

Status: Needs work » Needs review
FileSize
69.93 KB
761.47 KB

Updated the code:
- used the validatorbuilder
- removed translation and related classes for now
- changed constraint array notation to go with one and simple way only (as we can introduce the ListConstraint for having multiple constraints of the same type if needed), i.e.

$constraints = array(
  'Min' => 5,
  'Range' => array('min' => 5, 'max' => 10),
  'NotBlank' => array(),
);

Symfony interfaces/methods used so far.

Validation::createValidatorBuilder()
ValidatorBuilderInterface()
MetadataFactoryInterface
PropertyMetadataInterface
PropertyMetadataContainerInterface

So, we need to check whether we'll get @api tags on them with Symfony 2.2.

fago’s picture

FileSize
11.23 KB

and an interdiff

fago’s picture

Also, I'd love to hear more opinions on whether or not to follow symfony conventions with

  1. no class suffixes. As yched pointed out, we use $pluginType class suffixes, e.g. MinConstraint, MaxConstraint for our plugins. Symfony has the convention not do that, e.g. have Min, Max.
  2. replacement patterns. Symfony violation messages use {{ replacement }} patterns instead of our %replacement notation. However, the format_string() default is to go with %type replacements, so symfyony replacement patterns would work for us also - it's just about conventions.
    So we could
    • transform all messages to go with our pattern. That would add some runtime overhead for translating the generated patterns to ours though.
    • take over symfony patterns for given constraints only, define ours using %patterns. That would be inconsistent across constraints (symfony vs drupal) and burden translators with symfony replacements.
    • Adopt the symfony convention for replacement characters in violation messages. Gives as compatibility to upstream and consistency across constraints, but exposes another convention to developers and translators.

My opinion is that we should follow upstream conventions unlike there is a good reason not to, such that we ease cross-project collbaration and have a more consistent code-base. I'm not sure whether bugging developers and translators is enough reason to change conventions - but, as it would also avoid re-defining messages I'd prefer to go with symfony replacement patterns also.

Thoughts?

Gábor Hojtsy’s picture

I think its painful both from a developer point of view (DX) and translator point of view (TX) that a different pattern system is used here, especially that it has no hope to be widely adopted in Drupal core. Again I see this is a kind of question where yeah, 3rd party upstream compatibility and cross-collaboration would be great but not this close to freeze :/

Lars Toomre’s picture

Reading through this and related issue(s), I have the strong sense that the current Drupal translation system is more capable than what currently exists in Symfony. However, I am left wondering about what is practically involved in the items raised in #63.

Personally, I think Symphony is very limited in naming its classes such as Min and Max. In a complex system like Drupal, to me at least, it seems like we need some degree of qualification like MaxConstraint. As a naive developer, one could well implement a class like Max that might limit the maximum number to messages to retain in the {dblog} table (as an example). Such a class obviously has nothing to do with the Symphony/Drupal constraint system.

From the discussion in this issue, I have no sense of how many Symphony messages/classes might need to be overridden to work with Drupal's existing translation sub-system. Also, while it is admirable to want to contribute up-stream, is there a list of Drupal specific constraints that Symphony current lacks? Absent such a list, it seems that such contributions are a noble goal, but maybe something that should be left to D9 or D10 when there is more experience.

attiks’s picture

#63 I think we should respect Drupal standards, so use a suffix and use Drupal placeholders in the strings. Best way to do the latter is to extend the symfony class and override the message, there's another reason to do this, what if the message inside a symfony constraint is changed, and Drupal 8 adds this new version: if we don't override it it means all translations are gone?

#65 SF has like 30 Constraints defined, we can use 15 of them directly, the others depend on other SF components are not suitable (for the moment).

webmozart’s picture

Personally, I think Symphony is very limited in naming its classes such as Min and Max. In a complex system like Drupal, to me at least, it seems like we need some degree of qualification like MaxConstraint.

We usually also like to qualify our classes, but in this specific case, leaving away the "Constraint"-suffix had a reason. Constraints are not only classes, but they are at the same time used for configuration. For example:

use Symfony\Component\Validator\Constraints as Assert;

class Entity
{
    /** @Assert\Length(min = 3) */
    public $name;
}

As you can see, the annotation maps to the class, i.e.,

use Symfony\Component\Validator\Constraints\Length;

new Length(array('min' => 3))

is executed by the annotation parser. The same holds for the XML, YAML and PHP drivers, for example in PHP:

use Symfony\Component\Validator\Constraints\Length;

$classMetadata->addPropertyConstraint('name', new Length(array('min' => 3)))

or in YAML:

Vendor\Namespace\Model\Entity:
  properties:
    name:
      - Length: { min: 3 }

This way, custom constraints can be integrated without adapting the drivers or changing the DX. I strongly advise you to follow a similar path. Even if you register constraints through your plugin system, I suggest to let the keys of the configuration map to the constraints classes (instead of adding another layer of configuration that maps constraint names - such as "Length" - to constraint classes - such as "LengthConstraint"). fago proposed this in #25. This way you will maintain upstream compatibility and streamline the DX for devs using the Validator in different environments (Drupal and non-Drupal).

See also #45 for more information.

webmozart’s picture

#63 I think we should respect Drupal standards, so use a suffix and use Drupal placeholders in the strings. Best way to do the latter is to extend the symfony class and override the message

This will break compatibility between all non-Drupal constraints and Drupal constraints, i.e., each constraint will first have to be overridden by Drupal in order to be used by Drupal devs. Drupal constraints, on the other hand, will not be usable by non-Drupal devs. Again, I advise against that.

there's another reason to do this, what if the message inside a symfony constraint is changed, and Drupal 8 adds this new version: if we don't override it it means all translations are gone?

This won't happen. Symfony maintains a translation even for english messages. If we decide to reword the messages, we will adapt the translation, not the source.

attiks’s picture

Thanks for clarifying the translation part

yched’s picture

As I pointed earlier, I think the argument in #67 doesn't stand for the drupal use of the library.
Our config files do not refer to class names, but plugin ids, and plugin ids are mapped to actual class names through the Plugin API.
All our existing systems work that way, and the validation system should, too.

fago’s picture

All our existing systems work that way, and the validation system should, too.

It will and it already does (if you look at the patch). Still, we can have it work like that, while not doing the Constraint class suffix part to enable the annotation use of the classes. Considering someone is doing a Drupal8-Symfony application with some parts in Drupal and some parts in Symfony, being able to re-use the Drupal validators in Symfony entities might be desirable. Thus, I think it makes sense to leave out the Constriant suffix in this case.

For message replacements, I see a stronger point on using the Drupal pattern as directly influences DX. Moreover, the Drupal replacement syntax is compatible with what Symfony does, so it does not hinder the use of Drupal constraints in a Symfony environment. (It might make contributing it back a bit more difficult, but for that you'd have to do some small changes - think coding style - anyway).

Thus, I'd propose us to not use constraint suffixes in class names but to go with Drupal replacement characters in violation messages.

yched’s picture

It will and it already does (if you look at the patch)

Yes - except the plugin ids contained in the annotations are currently CamelCased, which is not what you'd expect in a plugin_id.

Also, I'm not hell bent on using the Constraint prefix on our classes just for the sake of it, but my point is that constraint classes used on the drupal side will need additional methods on top of Symfony classes, be it for UI configuration only.
So you won't be able to directly reuse a constraint class build for symfony directly in drupal anyway - or only a custom constraint class for a custom need on your site, that you know you'll never want to configure in a UI anyway.

fago’s picture

Issue tags: +form API

Yes, I'm not sure how UI-exposed validators will work exactly, but I don't expect us to expose every validator. This really has to be optional.

So you won't be able to directly reuse a constraint class build for symfony directly in drupal anyway - or only a custom constraint class for a custom need on your site, that you know you'll never want to configure in a UI anyway.

Still, I think keeping both system as close together as possible makes sense. First off, it helps people already knowing the other system to get into the new one.
But moreover, re-using Symfony constraints would be as easy as adding in the plugin definition or defining the constraint class plugin with a Drupaly message. The other way round, using an existing Drupal constraint from within a hybrid Symfony-Drupal or your custom symfony application would be straight forward (just import the class and use it).

fago’s picture

Issue tags: -form API +typed data
fago’s picture

Issue summary: View changes

Updated.

fago’s picture

fago’s picture

Issue summary: View changes

"tight" to "tied"

Status: Needs review » Needs work

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
71.28 KB
1.9 KB
737.47 KB

Fixed entity query relationships to account for the constraint rename.

YesCT’s picture

#77: d8_validation.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +field validation, +Entity forms, +typed data

The last submitted patch, d8_validation.patch, failed testing.

attiks’s picture

effulgentsia’s picture

Priority: Major » Critical

Postponed #1696648: [META] Untie content entity validation from form validation on this, so raising this one to critical. Please correct this if it's sensible to work on that issue prior to completing this one.

fago’s picture

#81: Works for me.

Now as the symfony-change went in and we figured out a way to handle translation at #1853096: Integrate symfony validation violations with Drupal translation I think we should re-roll this and #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3. to account for translations already.

For that all to happen we need to resolve #1852106: Add the symfony translator interface or translation component.

EclipseGc’s picture

FileSize
2.93 KB

OK, we're running short on time and I'm wondering why this has to be so complicated. I understand there's probably a bigger solution out there we're pursuing but I'd like to have at least some sort of validation NOW. I didn't try to get the validation running in tests yet, but I am actively using at least the String plugin, and spent a bit of time learning about the ways a lot of these could be validated. I didn't get all of them, but this should be a good first step.

Eclipse

fago’s picture

I don't think it's helpful to shot for a half-baked solution when the proper one has a patch that is basically ready. We have reached an agreement on how to move on with translation and the necessary improvements went into symfony validator, so we just need to do translation and get #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3. done.

I'm happy to take care of it and re-roll it tomorrow, howsoever we'll need to get in #1877632: Improve comments and clean-up code for EntityNG and the TypedData API first.

EclipseGc’s picture

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.phpundefined
@@ -0,0 +1,69 @@
+      case Primitive::FLOAT:
+        $valid = is_numeric($value);
+        break;
+      case Primitive::INTEGER:
+        // Also consider integer values represented as PHP string as valid.
+        $valid = ((string) (int) $value === (string) $value);

I suspect filter_var($value, FILTER_VALIDATE_FLOAT) and filter_var($value, FILTER_VALIDATE_INT) are better here?

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.phpundefined
@@ -0,0 +1,69 @@
+      case Primitive::URI:
+        // @todo: Support any URI which is valid according to rfc2396.
+        $valid = is_scalar($value) && valid_url($value, TRUE);

according to http://php.net/manual/en/filter.filters.validate.php filter_var($value, FILTER_VALIDATE_URL) should validate against rfc2396.

I see no validation of email address here which could also be done with filter_var($value, FILTER_VALIDATE_EMAIL). I dunno if that's an oversight or if it's in the patch later.

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -108,4 +126,144 @@ function create(array $definition, $value = NULL, array $context = array()) {
+  public function getValidationConstraintManager() {
+    if (!isset($this->constraintManager)) {
+      $this->constraintManager = drupal_container()->get('validation.constraint');
+    }
+    return $this->constraintManager;

This would be better done in the DIC. You can totally call this method when the TypedDataManager is added to the DIC and inject a reference to validation.constraint.

+++ b/core/lib/Drupal/Core/TypedData/Validation/MetadataFactory.phpundefined
@@ -0,0 +1,44 @@
+      throw new \InvalidArgumentException('The passed value must be a typed data object.');

Typically use statements are preferred to this, also in most other subsystems I've worked in we're extending the Exception and doing like a MetaDataException here instead. I don't really need a justification if you have a reasoning, but the use comment is definitely an issue.

+++ b/core/lib/Drupal/Core/TypedData/Validation/PropertyContainerMetadata.phpundefined
@@ -0,0 +1,55 @@
+      throw new \LogicException("There are no known properties.");

Same exception/use statement issue here.

+++ b/core/lib/Drupal/Core/Validation/ConstraintManager.phpundefined
@@ -0,0 +1,84 @@
+    $this->discovery = new SymfonyDiscoveryDecorator($this->discovery);

Guess I better go check this decorator out :-D

+++ b/core/lib/Drupal/Core/Validation/ConstraintManager.phpundefined
@@ -0,0 +1,84 @@
+    $this->discovery = new CacheDecorator($this->discovery, 'validation_constraints',  'cache');

If validation constraints are ever exposed through the UI you need per language caching so it should be:

$this->discovery = new CacheDecorator($this->discovery, 'validation_constraints:' . language(LANGUAGE_TYPE_INTERFACE)->langcode)

Also, the 3rd parameter for 'cache' is the default, so it's unnecessary.

+++ b/core/lib/Drupal/Core/Validation/SymfonyDiscoveryDecorator.phpundefined
@@ -0,0 +1,92 @@
+class SymfonyDiscoveryDecorator implements DiscoveryInterface {

OH, here it is. ok

+++ b/core/lib/Drupal/Core/Validation/SymfonyDiscoveryDecorator.phpundefined
@@ -0,0 +1,92 @@
+  public function getDefinitions() {
+    $definitions = $this->decorated->getDefinitions();
+
+    $definitions['Range'] = array(
+      'label' => t('Range'),
+      'class' => '\Symfony\Component\Validator\Constraints\Range',
+      'type' => array('integer', 'float'),
+    );
+    $definitions['Length'] = array(
+      'label' => t('Length'),
+      'class' => '\Symfony\Component\Validator\Constraints\Length',
+      'type' => 'string',
+    );
+    $definitions['Null'] = array(
+      'label' => t('Null'),
+      'class' => '\Symfony\Component\Validator\Constraints\Null',
+      'type' => FALSE,
+    );
+    $definitions['NotNull'] = array(
+      'label' => t('Not null'),
+      'class' => '\Symfony\Component\Validator\Constraints\NotNull',
+      'type' => FALSE,
+    );
+    $definitions['Blank'] = array(
+      'label' => t('Blank'),
+      'class' => '\Symfony\Component\Validator\Constraints\Blank',
+      'type' => FALSE,
+    );
+    $definitions['NotBlank'] = array(
+      'label' => t('Not blank'),
+      'class' => '\Symfony\Component\Validator\Constraints\NotBlank',
+      'type' => FALSE,
+    );
+
+    return $definitions;

This looks like you just needed the ability to manually set plugin definitions, I'd have encouraged a StaticDiscoveryDecorator that extends the existing StaticDiscovery class, and just accepts the decorated class in its constructor. A few quick calls to setDefinition() and you should be set.

xjm’s picture

+++ b/core/lib/Drupal/Core/TypedData/Validation/MetadataFactory.phpundefined
@@ -0,0 +1,44 @@
+      throw new \InvalidArgumentException('The passed value must be a typed data object.');

Typically use statements are preferred to this, also in most other subsystems I've worked in we're extending the Exception and doing like a MetaDataException here instead. I don't really need a justification if you have a reasoning, but the use comment is definitely an issue.

+++ b/core/lib/Drupal/Core/TypedData/Validation/PropertyContainerMetadata.phpundefined
@@ -0,0 +1,55 @@
+      throw new \LogicException("There are no known properties.");

Same exception/use statement issue here.

Actually, the first sentence is incorrect; we do not import PHP native/global classes like InvalidArgumentException with use. (So, new \InvalidArgumentException(); is correct). See the namespaces handbook page. However, a more specific typed exception could be a good idea.

fago’s picture

Status: Needs work » Needs review
FileSize
772.11 KB
68.95 KB

Thanks! It's great to get a review from a plugin system expert :-)

This would be better done in the DIC. You can totally call this method when the TypedDataManager is added to the DIC and inject a reference to validation.constraint.

Well, the idea here was to avoid instantiating the validation constraint manager if not needed. The typed data manager will be needed on pretty much every page request as it's needed for loading entities, but we won't need validation everywhere.

Typically use statements are preferred to this, also in most other subsystems I've worked in we're extending the Exception and doing like a MetaDataException here instead. I don't really need a justification if you have a reasoning, but the use comment is definitely an issue.

hm, regarding using "use:
Afaik it's ok to not use "use" if it's used only once, and I've seen it that way in other places also so I though it's ok. I must say I like it as using the exception just for one use is not as quick written as the simple global \. Howsoever, let's do what is considered best practice.
We probably should define a specific exception instead of the LogigException, but what's bad about using InvalidArgumentException? I think there are multiple places in core which use that and if it's just not respecting the intended usage/documentation of the class it fits very well imo.

@mail: Yep, the mail type went in after creating this patch. We'll need to add support for validating it.

ok, so here is an updated patch that fixes all conflicts to work again against the latest 8.x HEAD and the latest patch from #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3.. It does not address the review nor implement the TranslatorInterface yet.

Note: The code can be found in the validation-* branches of the enttiy api sandbox.

Status: Needs review » Needs work

The last submitted patch, d8_validation-both-patches.patch, failed testing.

Wim Leers’s picture

For completeness: there's now also a telephone field type, but it was agreed there that it's impossible to validate that, because there's no unified set of rules; the rules vary for each country. Hence there is no need to add support for validating that. (It was mostly added to leverage input[type=tel] on mobile devices, AFAICT; it's a specialized case of our text field type.)

fago’s picture

Status: Needs work » Needs review
FileSize
69.57 KB
772.72 KB

#87 missed to update a rather important reference to the Bundle-constraint - fixed that.

attiks’s picture

I quicly read the patch, it's looking good, I'll try to test some more during the weekend

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.phpundefined
@@ -0,0 +1,69 @@
+        $valid = is_scalar($value);

why not is_string?

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.phpundefined
@@ -0,0 +1,69 @@
+        $valid = is_scalar($value) && valid_url($value, TRUE);

why not is_string?

EclipseGc’s picture

FileSize
69.57 KB

OK, I did not review this patch at length, I just applied what was here, unapplied the issue this depended on and then did a git pull/diff. The size looks right though, so I'm fairly confident this is a good starting point. Let's see how tests fair.

Eclipse

EclipseGc’s picture

FileSize
69.76 KB

Well, the idea here was to avoid instantiating the validation constraint manager if not needed. The typed data manager will be needed on pretty much every page request as it's needed for loading entities, but we won't need validation everywhere.

Yeah, but if you ever need it, even once, then it's going to be there. Is it so unlikely to be used that it's not worth scripting this in the DIC? I went ahead and did so in this patch just to prove the point. We may need to throw an exception in the getter for it though. I think once this and the context system are both in, this is really really likely to see some use.

hm, regarding using "use:

hmm indeed. xjm's post leads me to believe I'm wrong. I chose not to touch it in this patch.

Notes of interest:

I went ahead and created the StaticDiscoveryDecorator. I had to do a little more work than I anticipated but it wasn't bad. I frankly kind of like it. We should get testing for it in a follow up issue. This is especially nice because all the symfony definitions you wanted that were already specific to this validation plugin type now exist in the constructor on the manager. That seems like an epic win to me since there's no digging through discovery classes to figure out wtf these came from.

I updated the cache decorator per my own statements, however it looks like we may need to take it a tad further in light of: #1893818: Plugin discovery caches of blocks are not cleared for all languages This is one more argument that we should be investing time in a generic PluginManager that everyone can use w/o needing to know how to set it up as far as I'm concerned, but that's probably something we can do later if time permits. Still worth looking at.

I replaced all of the places I said could use filter_var() with filter_var() calls. This was only problematic for integers since filter_var() returns what was passed to it if it validates, and the test was passing 0 (good thing). In this one case I'm returning TRUE/FALSE. If that's not OK then we should discuss it further. I did not try to get coverage on the other TypedData types that were not already in this patch, so that still needs to be done I think.

This is passing the TypedData tests locally for me, so I'm hoping it passes the whole test suite.

Eclipse

Status: Needs review » Needs work
Issue tags: -field validation, -Entity forms, -typed data

The last submitted patch, validate.patch, failed testing.

EclipseGc’s picture

Status: Needs work » Needs review
Issue tags: +field validation, +Entity forms, +typed data

#93: validate.patch queued for re-testing.

EclipseGc’s picture

Issue summary: View changes

Updated issue summary.

EclipseGc’s picture

I went ahead and added a follow up to this issue summary that fixes a bunch of issues with the TypedDataManager/Factory and Base classes. Most of these changes are pretty straight forward and a couple even have existing todos in the code. Since they were all related to the approach that Plugins expects, I just went ahead and fixed all of them together since it's a small patch and it'll make more sense this way. #1899950: Make TypedData plugins conform better to the Plugin Interfaces

Eclipse

fubhy’s picture

+++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticDiscoveryDecorator.phpundefined
@@ -0,0 +1,54 @@
+/**
+ * @file
+ * Contains \Drupal\Component\Plugin\Discovery\StaticDiscoveryDecorator.
+*/
+
+namespace Drupal\Component\Plugin\Discovery;
+
+/**
+ * A decorator that allows manual registration of undiscoverable definitions.
+ */
+class StaticDiscoveryDecorator extends StaticDiscovery {
+
+  /**
+   * The Discovery object being decorated.
+   *
+   * @var \Drupal\Component\Plugin\Discovery\DiscoveryInterface

I like what I see... That's much better than a symfony specific discovery. It's not really a discovery anyways but rather a registration procedure.

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/BundleConstraint.phpundefined
@@ -0,0 +1,68 @@
+ * @Plugin(
+ *   id = "Bundle",
+ *   label = @Translation("Bundle", context = "Validation"),
+ *   type = "entity"
+ * )

What's the reason to go with CamelCased plugin id's here? From reading through this patch and the previous versions of it (including the symfony validator component) I can't really find a good explanation on why we would have to introduce a special case here. Special cases are horrible for DX :).

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/BundleConstraintValidator.phpundefined
@@ -0,0 +1,28 @@
+    $entity = isset($typed_data) ? $typed_data->getValue() : FALSE;

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/EntityTypeConstraintValidator.phpundefined
@@ -0,0 +1,28 @@
+    $entity = isset($typed_data) ? $typed_data->getValue() : FALSE;

I am wondering what the isset() check is for? In which case do we have to protect for ->validate(NULL, $constraint)? If there is no good reason for that we are basically covering up false invocations here.

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/EntityTypeConstraint.phpundefined
@@ -0,0 +1,55 @@
+
+use Symfony\Component\Validator\Constraint;
+use Drupal\Core\Annotation\Plugin;
+use Drupal\Core\Annotation\Translation;
+
+
+/**
+ * Checks if a value is a valid entity type.
+ *
+ * @todo: Move this below the entity core component.

There are two empty lines there after the last use statement.

fubhy’s picture

+++ b/core/lib/Drupal/Core/TypedData/Validation/Metadata.phpundefined
@@ -0,0 +1,94 @@
\ No newline at end of file

Newline!

+++ b/core/lib/Drupal/Core/TypedData/Validation/PropertyContainerMetadata.phpundefined
@@ -0,0 +1,55 @@
\ No newline at end of file

One more!

fago’s picture

That seems like an epic win to me since there's no digging through discovery classes to figure out wtf these came from.

That looks good, but it's outside of the caching layer now, not? I'd prefer not running through all the setDefinition() call each time the constraint manager is constructed - i.e. it's now created on pretty much every page load given the optional dependency has been changed also.

I am wondering what the isset() check is for? In which case do we have to protect for ->validate(NULL, $constraint)? If there is no good reason for that we are basically covering up false invocations here.

That's how symfony validator handles NULL values - they run through all constraint validators and it's up the constraint to show an error or now. The NotNull or NotBlank validator do, others usually don't.

What's the reason to go with CamelCased plugin id's here? From reading through this patch and the previous versions of it (including the symfony validator component) I can't really find a good explanation on why we would have to introduce a special case here. Special cases are horrible for DX :).

Well, the intention here is to be as close to Symfony validator as possible. Symfony validator directly uses the Constraint classes to configure validation constraints, e.g. via Annotations - or also in their Yaml variant where the do away with the namespaces for readability as well. E.g. this is a part of a symfony validation constraint specification in yml:

    firstName:
      # Constraint without value
      - NotNull: ~
      # Constraint with single value
      - Range:
          min: 3

I think we should follow that such that developers that already now symfony validator immediately recognize our array-config to be equivalent constraint configuration.
Related, I could see us picking this up in other places as well, e.g. to improve DX for typed data classes as well (or even all plugins), see #1867880: Bring data type plugins closer to their PHP classes - but that's a different issue. The reasoning here is to play well with what symfony already does.

$valid = is_scalar($value);

why not is_string?

Because a PHP integer of 3 is also a valid string, added a comment.

I replaced all of the places I said could use filter_var() with filter_var() calls. This was only problematic for integers since filter_var() returns what was passed to it if it validates, and the test was passing 0 (good thing).

yes, but 0 is a valid float also - fixed that.

ad #98: fixed.

Yeah, but if you ever need it, even once, then it's going to be there. Is it so unlikely to be used that it's not worth scripting this in the DIC? I went ahead and did so in this patch just to prove the point. We may need to throw an exception in the getter for it though. I think once this and the context system are both in, this is really really likely to see some use.

Well, on an average entity_load() the typed data manager will be needed, but not the validation constraint manager. Thus, if we do so we need at least make sure there is not much weight in the class being constructed. Does it needlessly fetch the cache also?
The previous code was inspired with what symfony does for optional dependenies, thus you could inject your own manager while it defaulted to pulling it from the DIC. Yes, not 100% optimal, but it works efficiently and is injectable.

fago’s picture

ok, implemented validation according the discussion over at #1853096: Integrate symfony validation violations with Drupal translation -> see the interdiff.

What's left here is imho only
- implementation for the new email field/type
- resolving constraint-manager instantation?

fago’s picture

And now without debug statement ;-)

fago’s picture

Found two issues and fixed them.

EclipseGc’s picture

That seems like an epic win to me since there's no digging through discovery classes to figure out wtf these came from.

That looks good, but it's outside of the caching layer now, not? I'd prefer not running through all the setDefinition() call each time the constraint manager is constructed - i.e. it's now created on pretty much every page load given the optional dependency has been changed also.

No, it's not outside the caching layer, it will will set the definitions on class construction and they will be set in the StaticDiscoveryDecorator's definition variable. When getDefinition/s() is called the annotations will be parsed, and on up until we hit the static decorator, it will then add it's registered definitions to the existing list which will finally hit cache and be cached. Your point about setting these definitions needlessly on construct is well taken though. We can move this to a protected setDefinitions() method on the manager, and call to it from inside getDefinition/s() on the manager.

Well, on an average entity_load() the typed data manager will be needed, but not the validation constraint manager. Thus, if we do so we need at least make sure there is not much weight in the class being constructed. Does it needlessly fetch the cache also?

No, it should not. The cache will only be fetched on a getDefinition/s() call. Otherwise you just have the weight of exactly what you see in the constructor, which is a handful of class instantiations.

The previous code was inspired with what symfony does for optional dependenies, thus you could inject your own manager while it defaulted to pulling it from the DIC.

The getters and setters are still there, there's nothing that prevents exactly that behavior as far as I can tell, we're just injecting the default through the DIC now instead of manually requesting it from the DIC if one was not given. We should likely have an exception in the getter if one is not set (for whatever reason).

And we still need email validation. Otherwise I think we're really really close here.

Eclipse

EclipseGc’s picture

FileSize
77.13 KB

OK, I moved the definitions here into a protected registerDefinitions() method on the plugin manager. If you dislike this we have one more option for doing this that involves extending the existing Symfony classes into Classes in a plugin directory and just adding annotations to dummy extended classes. Either way, this seems like a pretty straight forward way to fix this, and I like this notion of a StaticDiscoveryDecorator.

I added some validation to the email type, but the tests should probably be expanded. Not sure I did this the way you want, but I figured I could take a stab at it. Gotta be better than the todo. :-)

Eclipse

EclipseGc’s picture

FileSize
2.54 KB

and the interdiff

Status: Needs review » Needs work

The last submitted patch, validate.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
8.23 KB
78.86 KB

... Your point about setting these definitions needlessly on construct is well taken though. We can move this to a protected setDefinitions() method on the manager, and call to it from inside getDefinition/s() on the manager.

I see - that sounds good.

+++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
@@ -73,6 +79,28 @@ public function __construct() {
+    if (!$this->definitions) {
+      $this->registerDefinitions();
+      $this->definitions = $this->discovery->getDefinitions();
+    }

hm, this potentially introduces a stall cache - we should not re-introduce a static outside of the respective decorator.

I reworked registering definitions to work via a callback that's passed to the decorator - so the decorator is easily re-usable and the definitions are clearly listed in the manager class.

I also added support for e-mail validation - fortunately there is already a symfony constraint we can use (it uses filter_var() also). See attached interdiff and patch.

fago’s picture

The getters and setters are still there, there's nothing that prevents exactly that behavior as far as I can tell, we're just injecting the default through the DIC now instead of manually requesting it from the DIC if one was not given. We should likely have an exception in the getter if one is not set (for whatever reason).

With parsing registering-definitions.code only when validation constraint definitions are requested and not cached, instantiating the ConstraintManager class should not hurt any more - so I think it should be ok to inject it via the DIC now.

The exception is something we still might want to add, not sure which one though?

And we still need email validation. Otherwise I think we're really really close here.

As this is done now too, the patch is complete and so far ready.

Status: Needs review » Needs work

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
79.8 KB
968 bytes

There is a new test for the field-type which seems to be broken, not sure how this passed tests previously but anyhow, here is a fix.

Status: Needs review » Needs work
Issue tags: -field validation, -Entity forms, -typed data

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
Issue tags: +field validation, +Entity forms, +typed data

#110: d8_validation.patch queued for re-testing.

fago’s picture

Attached is a patch which improves registerDefinitions() to go with $this->discovery instead of receiving an extra $discovery argument. That cuts it also and is less complex.

Status: Needs review » Needs work
Issue tags: -field validation, -Entity forms, -typed data

The last submitted patch, d8_validation.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
Issue tags: +field validation, +Entity forms, +typed data

#113: d8_validation.patch queued for re-testing.

fago’s picture

After further discussion with EclipseGC on IRC we came to the agreement to further work on a clean solution for adding in the symfony definitions in a follow-up, such that we can move on with validation here.

So then, this should be ready. Any reviews?

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Random fails, looks good to me

RTBC

fago’s picture

Created the according potx issue for extracting violation messages: #1903362: Support for Drupal 8 validation constraint messages

fubhy’s picture

+1 RTBC! Good job everyone :)

attiks’s picture

@fago thanks for bringing this home, nice work!

effulgentsia’s picture

Looks good. Just some nits, but IMO, they can be follow up.

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/BundleConstraint.php
@@ -0,0 +1,68 @@
+  /**
+   * The bundle option.
+   *
+   * @var string|array
+   */
+  public $bundle;
+
+  /**
+   * Gets the bundle option as array.
+   *
+   * @return array
+   */
+  public function getBundleOption() {
+    // Support passing the bundle as string, but force it to be an array.
+    if (!is_array($this->bundle)) {
+      $this->bundle = array($this->bundle);
+    }
+    return $this->bundle;
+  }

Can we instead just have $bundles, and require it to always be an array?

+++ b/core/lib/Drupal/Core/Plugin/Validation/Constraint/LengthConstraint.php
@@ -0,0 +1,39 @@
+/**
+ * Length constraint.
+ *
+ * Overrides the symfony constraint to use Drupal-style replacement patterns.

Given that we have DrupalTranslator that could do the replacements from {{ foo }} to %foo, why do we need this override?

+++ b/core/lib/Drupal/Core/TypedData/Type/Date.php
@@ -40,9 +40,13 @@ public function setValue($value) {
+  /**
+   * Overrides TypedDataInterface::getString().
+   */
+  public function getString() {
+    return (string) $this->getValue();
+  }

Looks like the base class does the same thing, so can be removed from here?

+++ b/core/lib/Drupal/Core/TypedData/Type/Duration.php
@@ -32,21 +32,29 @@ class Duration extends TypedData {
+    try {

Do we need the try/catch here? If so, please add a comment as to why.

+++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
@@ -0,0 +1,120 @@
+    // We overrode getDefinition() and getDefinitions() so we must pass the
+    // PluginManager and not just discovery.

This comment no longer appears true.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
@@ -504,6 +504,52 @@ public function getContainedStrings(TypedDataInterface $wrapper, $depth, array &
+      'constraints' => array(
+        'EntityType' => 'node',
+        'Bundle' => 'article',
+      ),
+      'label' => t('Test entity'),

The label would make more sense as 'Test node', right? Also, I thought we weren't supposed to use t() in tests.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
@@ -504,6 +504,52 @@ public function getContainedStrings(TypedDataInterface $wrapper, $depth, array &
+    node_save($node);

$node->save() would be better.

+++ b/core/modules/system/system.api.php
@@ -163,6 +163,8 @@ function hook_cron() {
+ *   - constraints: An array of validation constraints for this type. See
+ *     \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.

Thanks for adding this to the API docs. Since the example below this is an email type, can we add this into there as well?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationUITest.php
@@ -39,7 +39,7 @@ function testTranslationUI() {
-      $this->assertIdentical($stored_value, $value, $message);
+      $this->assertEqual($stored_value, $value, $message);

Why are we making this less strict? Could we instead explicitly cast what needs to be cast?

-4 days to next Drupal core point release.

fago’s picture

Can we instead just have $bundles, and require it to always be an array?

That would probably make sense as it would make it easier and more reliable to directly read the bundle from the constraint array also. But let's leave this to a follow-up.

Given that we have DrupalTranslator that could do the replacements from {{ foo }} to %foo, why do we need this override?

This is the message we are going to translate, whereas the translator cares about the provided replacement arguments to actually be %foo.

Why are we making this less strict? Could we instead explicitly cast what needs to be cast?

This was necessary as the typed-data classes do not casts any more, but leave the input-data as is and leave it up to the validation layer to decide whether it's ok. Adding a cast in the test would not improve test coverage, but anyhow I think it's ok that way as we generally do not care whether the storage layer loads an integer as PHP integer or PHP string.

fago’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.16 KB
80.04 KB

Attached patch address the other points raised.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

#122 + #123 looks good, assuming all tests will pass

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
80.04 KB

Rerolled for #1801304-324: Add Entity reference field. CNR for bot.

fago’s picture

Status: Needs review » Reviewed & tested by the community

@effulgentsia: You've beaten me by 3 minutes ;-)

Re-roll looks good, back to RTBC given bot stays green.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

An important one as this will allow progress on Web Services, Entity NG and SCOTCH.

Thanks all!

fago’s picture

fago’s picture

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

Anonymous’s picture

stefan.r’s picture

Issue summary: View changes
Status: Closed (fixed) » Needs review
FileSize
928 bytes

Small bug in the violation message.

yched’s picture

Status: Needs review » Closed (fixed)

@stefan.r: Thanks!
We try to avoid reopening long-closed issues for small fixes. Instead, could you post the patch in a separate issue, and link back from here ?