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
- Implement translating violation messages as discussed at #1853096: Integrate symfony validation violations with Drupal translation
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
- Various cleanups of the TypedDataManager/Factory and base classes. #1899950: Make TypedData plugins conform better to the Plugin Interfaces
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:
- 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.
- 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
- 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
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
Related issues
- #1696648: [META] Untie content entity validation from form validation
- #1797438: HTML5 validation is preventing form submit and not fully accessible
- #742344: Allow forms to set custom validation error messages on required fields
- #1493324: Inline form errors for accessibility and UX
- #52051: Validation API/UI
- #471264: Consistently name validation/sanitation functions
- #478758: DX: Allow ordering of #validate and #submit callbacks
- #1829420: Validate #required, #maxlength and #options in standard #element_validate handlers
- #567104: Should entities perform field validation if FAPI validation fails?
- #1218814: PDOException because of incorrect validation of number fields
- #93447: Deleting the first value in a required, multivalue field fails validation
- #57518: drupal 4.8: need to decouple node validation from forms
- more: http://drupal.org/project/issues/drupal?text=validate&version=8.x
Comment | File | Size | Author |
---|---|---|---|
#131 | drupal8.bundle-validation-message.1845546-131.patch | 928 bytes | stefan.r |
#125 | d8_validation-125.patch | 80.04 KB | effulgentsia |
#123 | d8_validation.patch | 80.04 KB | fago |
#123 | d8_validation.interdiff.txt | 4.16 KB | fago |
#113 | d8_validation.patch | 79.79 KB | fago |
Comments
Comment #1
attiks CreditAttribution: attiks commentedFirst thing we need to figure out is what needs to be comitted before feature freeze, and what can be done after?
Comment #1.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #1.1
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #1.2
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #2
pounardGetting SF validation API/Component first would be all of the first, biggest and easiest step to begin with.
Comment #3
attiks CreditAttribution: attiks commented@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.
Comment #4
fagoMy 2 cents:
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.
Comment #5
attiks CreditAttribution: attiks commentedIt 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.
Comment #6
fagoWe have typed data definitions for the field level also? See fieldDefinitions() in the storage controller.
Comment #6.0
fagoUpdated issue summary.
Comment #7
attiks CreditAttribution: attiks commentedSwitching 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, ...
Comment #8
webchickI 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?
Comment #9
webchickBut in general, this is a new major API, so would be bound to feature freeze.
Comment #9.0
webchickUpdated issue summary.
Comment #10
attiks CreditAttribution: attiks commentedI'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.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedAt 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.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedIf #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.
Comment #13
webchickIf 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.
Comment #14
fagoUpdate: 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.
Comment #15
fagoAs 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..
Comment #17
fagoUpdated the patch. This is already quite complete.
Summary
Patch notes
Todo:
Follow-ups:
Comment #19
fagoFixed the test fail.
Comment #20
attiks CreditAttribution: attiks commentedNice work, I had a quick dreditor look and this looks amazing, I'll have a closer look later today.
Comment #21
yched CreditAttribution: yched commentedI only skimmed through this for now, just a couple remarks :
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 ?
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.
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.
Comment #22
fagoGood 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?
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?
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.
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.
Comment #22.0
fagoUpdated issue summary.
Comment #22.1
fagoUpdated issue summary.
Comment #23
fagoI updated the issue summary based upon #17.
Comment #24
yched CreditAttribution: yched commentedre: 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.
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.
Comment #25
fagoTrue, but it should be optional also. I'll fix it that way.
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
by default, but allow overriding it via the 'constraint' key:
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.
Comment #26
tim.plunkettThis 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 :)
Comment #27
yched CreditAttribution: yched commentedre #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.
Comment #28
attiks CreditAttribution: attiks commented#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.
Comment #29
fagoI 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?
Comment #30
fagoAttaching an interdiff to #19.
Comment #31
yched CreditAttribution: yched commentedI'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 :-)
Comment #32
attiks CreditAttribution: attiks commentedQuick question: why don't we use Drupal placeholders in here?
Comment #33
fagoWe 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?
Comment #34
attiks CreditAttribution: attiks commentedSince we wrap them already it probably is the best to use drupal placeholders, so it's consistent.
Comment #35
fagoWhile 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?
Comment #36
attiks CreditAttribution: attiks commentedIn 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.
Comment #37
fagoI 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.
Comment #38
fagoAttached patch implements the following todo items:
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.
Comment #39
Lars Toomre CreditAttribution: Lars Toomre commented@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.
Comment #41
fagoI 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.
Comment #42
fagoMy 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.
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 :)
Comment #43
xjmWhile 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!
Comment #44
Lars Toomre CreditAttribution: Lars Toomre commentedRegarding #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.
Comment #45
webmozart CreditAttribution: webmozart commentedWe 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.
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):
(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.
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.
Comment #46
yched CreditAttribution: yched commentedThing 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 ?
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.
Comment #48
webmozart CreditAttribution: webmozart commentedI 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
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".
Comment #49
webmozart CreditAttribution: webmozart commentedI also recommend to use
for constructing the validator. The constructor of the Validator class is not part of our public API, but Validation is.
Comment #50
attiks CreditAttribution: attiks commented#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)
Comment #51
fagoAtually, we can directly go with the { var } notation also, that works with our translation system / t().
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.
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.
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!
Comment #52
fagoRe-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?
Comment #53
attiks CreditAttribution: attiks commented#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.Comment #54
fagoad #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.
Comment #55
attiks CreditAttribution: attiks commentedIRC discussion with fago, added for reference
Comment #56
yched CreditAttribution: yched commentedFWIW, 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 :-/]
Comment #57
attiks CreditAttribution: attiks commented#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.
Comment #58
fagoActually, 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.
Comment #59
attiks CreditAttribution: attiks commented#52 Translation can be added in a follow-up
Comment #60
fagoopened #1853096: Integrate symfony validation violations with Drupal translation for figuring out details on translation.
Comment #61
fagoUpdated 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.
Symfony interfaces/methods used so far.
So, we need to check whether we'll get @api tags on them with Symfony 2.2.
Comment #62
fagoand an interdiff
Comment #63
fagoAlso, I'd love to hear more opinions on whether or not to follow symfony conventions with
So we could
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?
Comment #64
Gábor HojtsyI 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 :/
Comment #65
Lars Toomre CreditAttribution: Lars Toomre commentedReading 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.
Comment #66
attiks CreditAttribution: attiks commented#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).
Comment #67
webmozart CreditAttribution: webmozart commentedWe 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:
As you can see, the annotation maps to the class, i.e.,
is executed by the annotation parser. The same holds for the XML, YAML and PHP drivers, for example in PHP:
or in YAML:
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.
Comment #68
webmozart CreditAttribution: webmozart commentedThis 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.
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.
Comment #69
attiks CreditAttribution: attiks commentedThanks for clarifying the translation part
Comment #70
yched CreditAttribution: yched commentedAs 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.
Comment #71
fagoIt 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.
Comment #72
yched CreditAttribution: yched commentedYes - 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.
Comment #73
fagoYes, 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.
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).
Comment #74
fagoComment #74.0
fagoUpdated.
Comment #75
fagoRe-rolled the patch using the latest patch from #1849564: Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3..
Comment #75.0
fago"tight" to "tied"
Comment #77
fagoFixed entity query relationships to account for the constraint rename.
Comment #78
YesCT CreditAttribution: YesCT commented#77: d8_validation.patch queued for re-testing.
Comment #80
attiks CreditAttribution: attiks commentedFYI: https://github.com/symfony/symfony/pull/6137 is merged!
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedPostponed #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.
Comment #82
fago#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.
Comment #83
EclipseGc CreditAttribution: EclipseGc commentedOK, 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
Comment #84
fagoI 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.
Comment #85
EclipseGc CreditAttribution: EclipseGc commentedI suspect filter_var($value, FILTER_VALIDATE_FLOAT) and filter_var($value, FILTER_VALIDATE_INT) are better here?
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.
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.
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.
Same exception/use statement issue here.
Guess I better go check this decorator out :-D
If validation constraints are ever exposed through the UI you need per language caching so it should be:
Also, the 3rd parameter for 'cache' is the default, so it's unnecessary.
OH, here it is. ok
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.
Comment #86
xjmActually, 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.Comment #87
fagoThanks! It's great to get a review from a plugin system expert :-)
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.
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.
Comment #89
Wim LeersFor 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.)Comment #90
fago#87 missed to update a rather important reference to the Bundle-constraint - fixed that.
Comment #91
attiks CreditAttribution: attiks commentedI quicly read the patch, it's looking good, I'll try to test some more during the weekend
why not is_string?
why not is_string?
Comment #92
EclipseGc CreditAttribution: EclipseGc commentedOK, 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
Comment #93
EclipseGc CreditAttribution: EclipseGc commentedYeah, 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.
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
Comment #95
EclipseGc CreditAttribution: EclipseGc commented#93: validate.patch queued for re-testing.
Comment #95.0
EclipseGc CreditAttribution: EclipseGc commentedUpdated issue summary.
Comment #96
EclipseGc CreditAttribution: EclipseGc commentedI 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
Comment #97
fubhy CreditAttribution: fubhy commentedI 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.
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 :).
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.
There are two empty lines there after the last use statement.
Comment #98
fubhy CreditAttribution: fubhy commentedNewline!
One more!
Comment #99
fagoThat 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.
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.
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:
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.
Because a PHP integer of 3 is also a valid string, added a comment.
yes, but 0 is a valid float also - fixed that.
ad #98: fixed.
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.
Comment #100
fagook, 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?
Comment #101
fagoAnd now without debug statement ;-)
Comment #102
fagoFound two issues and fixed them.
Comment #103
EclipseGc CreditAttribution: EclipseGc commentedNo, 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.
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 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
Comment #104
EclipseGc CreditAttribution: EclipseGc commentedOK, 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
Comment #105
EclipseGc CreditAttribution: EclipseGc commentedand the interdiff
Comment #107
fagoI see - that sounds good.
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.
Comment #108
fagoWith 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?
As this is done now too, the patch is complete and so far ready.
Comment #110
fagoThere 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.
Comment #112
fago#110: d8_validation.patch queued for re-testing.
Comment #113
fagoAttached 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.
Comment #115
fago#113: d8_validation.patch queued for re-testing.
Comment #116
fagoAfter 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?
Comment #117
EclipseGc CreditAttribution: EclipseGc commentedRandom fails, looks good to me
RTBC
Comment #118
fagoCreated the according potx issue for extracting violation messages: #1903362: Support for Drupal 8 validation constraint messages
Comment #119
fubhy CreditAttribution: fubhy commented+1 RTBC! Good job everyone :)
Comment #120
attiks CreditAttribution: attiks commented@fago thanks for bringing this home, nice work!
Comment #121
effulgentsia CreditAttribution: effulgentsia commentedLooks good. Just some nits, but IMO, they can be follow up.
Can we instead just have $bundles, and require it to always be an array?
Given that we have DrupalTranslator that could do the replacements from {{ foo }} to %foo, why do we need this override?
Looks like the base class does the same thing, so can be removed from here?
Do we need the try/catch here? If so, please add a comment as to why.
This comment no longer appears true.
The label would make more sense as 'Test node', right? Also, I thought we weren't supposed to use t() in tests.
$node->save() would be better.
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?
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.
Comment #122
fagoThat 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.
This is the message we are going to translate, whereas the translator cares about the provided replacement arguments to actually be %foo.
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.
Comment #123
fagoAttached patch address the other points raised.
Comment #124
attiks CreditAttribution: attiks commented#122 + #123 looks good, assuming all tests will pass
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedRerolled for #1801304-324: Add Entity reference field. CNR for bot.
Comment #126
fago@effulgentsia: You've beaten me by 3 minutes ;-)
Re-roll looks good, back to RTBC given bot stays green.
Comment #127
Dries CreditAttribution: Dries commentedCommitted to 8.x.
An important one as this will allow progress on Web Services, Entity NG and SCOTCH.
Thanks all!
Comment #128
fagoThanks!
Opened the follow-up as discussed #1905620: Enforce the BundleConstraint "bundle" option to be always an array
Let's use this with #1696648: [META] Untie content entity validation from form validation now.
Comment #129
fagoopened #1913334: Support choice based validation
Comment #130.0
(not verified) CreditAttribution: commentedAdded a follow up task to #1899950: Make TypedData plugins conform better to the Plugin Interfaces
Comment #131
stefan.r CreditAttribution: stefan.r commentedSmall bug in the violation message.
Comment #132
yched CreditAttribution: yched commented@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 ?