Upgrade validator integration for Symfony versions 2.5+

It is unlikely that it will disappear in 2.x, at least. Symfony 2.3 onwards uses semver, so backward compatibility will be there. But let's do this anyway. I think this is related to #2343043: valid_email_address() should use egulias/EmailValidator and become deprecated.

I am putting it back to Active since #2278353: Update to Symfony 2.5 has gone in.

I can confirm that the BC mode won't disappear in the 2.x line. Symfony 3.0, however, won't feature that mode.

Is there anything I can do to help?

@cilefen: That's correct. However, no new features will be added to the 2.4 API. Just be aware of that.

@cilefen If you do not set a version, the API version will switch between 2.4 (for PHP < 5.3.9) and 2.5-BC (for PHP > 5.3.9).

The 2.5-BC API uses classes implementing multiple interfaces (2.4+2.5) with overlapping method definitions. This wasn't supported in PHP < 5.3.9. Hence you need to set the version to 2.5 manually if you want to use the 2.5 API also on PHP < 5.3.9.

@cilefen Well:

• The 2.5-BC API may be a bit slower, but allows to use constraints designed for the 2.4 API.
• The 2.5 API may be slightly faster due to the removed BC layer.

Since Drupal 8 is not released yet, the BC argument (pro 2.5-BC API) should not really apply. The performance difference (pro 2.5 API) should be minimal, but you better measure it for yourselves. So you can take any of them, really.

Need automatic patch testing

Need to fix tests after upgrade_validator-2343035-17.patch

BTW
I recommend starting resolve this issue from FieldValidationTest

[01:35 PM]-[root@precise64]-[/var/www/drupal]-[git master] 
# drush test-run Drupal\\field\\Tests\\FieldValidationTest --uri="http://drupal.192.168.56.133.xip.io" --methods="testFieldConstraints" --verbose
Initialized Drupal 8.0.0-dev root directory at /var/www/drupal                                                                          [notice]
Initialized Drupal site drupal.192.168.56.133.xip.io at sites/default                                                                   [notice]
Executing: mysql --defaults-extra-file=/tmp/drush_sYZfKd --database=drupal8 --host=127.0.0.1 --silent  < /tmp/drush_tjk3YJ
Loading outputformat engine.                                                                                                            [notice]
Starting test Drupal\field\Tests\FieldValidationTest.                                                                                [ok]
Drupal\field\Tests\FieldValidationTest 6[2] пройдено, 1 провален, 1 исключение                             [error]
Test Symfony\Component\Validator\Validator\RecursiveContextualValidator->validateClassNode() failed: Invalid argument supplied for   [error]
foreach()<pre class="backtrace">Symfony\Component\Validator\Validator\RecursiveContextualValidator->validateClassNode(Object,
'000000006149551300000000529cc498', Object, '', Array, NULL, 1, Object)
Symfony\Component\Validator\Validator\RecursiveContextualValidator->validateObject(Object, '', Array, 1, Object)
Symfony\Component\Validator\Validator\RecursiveContextualValidator->validate(Object, NULL, NULL)
Symfony\Component\Validator\Validator\RecursiveValidator->validate(Object)
Drupal\Core\TypedData\TypedData->validate()
Drupal\field\Tests\FieldValidationTest->testFieldConstraints()
Drupal\simpletest\TestBase->run(Array)
simpletest_drush_run_test('Drupal\field\Tests\FieldValidationTest')
drush_test_run('Drupal\field\Tests\FieldValidationTest')
call_user_func_array('drush_test_run', Array)
_drush_invoke_hooks(Array, Array)
drush_command('Drupal\field\Tests\FieldValidationTest')
call_user_func_array('drush_command', Array)
drush_dispatch(Array)
Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
drush_main()
</pre> in /var/www/drupal/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php on
line 572
Test Drupal\field\Tests\FieldValidationTest->testFieldConstraints() failed: Value array (                                            [error]
) is equal to value array (
  &#039;0.value&#039; =&gt; 
  array (
    0 =&gt; &#039;&lt;em class=&quot;placeholder&quot;&gt;kNBgHiAc_label&lt;/em&gt; does not accept the value -1.&#039;,
  ),
  &#039;2.value&#039; =&gt; 
  array (
    0 =&gt; &#039;&lt;em class=&quot;placeholder&quot;&gt;kNBgHiAc_label&lt;/em&gt; does not accept the value -1.&#039;,
  ),
  &#039;3.value&#039; =&gt; 
  array (
    0 =&gt; &#039;&lt;em class=&quot;placeholder&quot;&gt;kNBgHiAc_label&lt;/em&gt; does not accept the value -1.&#039;,
  ),
). in /var/www/drupal/core/modules/field/src/Tests/FieldValidationTest.php on line 99
Command dispatch complete  

@cilefen Ok, Thank you for suggestions :)

Can I do anything to help? If you want, we can have a phone call to resolve your issues.

All external library updates should be treated as critical during the beta, otherwise we have very little chance of being able to use or contribute back upstream bug fixes and performance issues once 8.0.0 is out.

I'm also hoping we can update at least some libraries in minor versions assuming no major bc breaks, and that absolutely depends on not using old/deprecated versions of things beforehand.

I am giving this a shot. There are many interfaces changed, as mentioned in #32. The factory interface has also changed. I do not have an interdiff as I went about this separately. This is mainly because the patch has all method implementations in ClassMetadata and none in Metadata. I thought it would be simpler to just derive what I needed and start fresh.

I have given some defaults to cascading and traversal strategy. I will need some help there. I will be pretty surprised if the patch passes, but it's a start.

@hussainweb If you need help, contact me at bschussek@gmail.com. I'm the developer of the Validator component.

Another attempt. I have no reason to do this but I am curious to see if installer moves ahead, at least, when there is no exception.

Thank you for the offer to help @webmozart. I am trying to understand this more. I have sent you an email with more details.

Thank you for your work @hussainweb

As far as I understand the BC layer will exist until symfony 3.0, which we won't use for Drupal 8 at least, given the changes.
We will probably either stick with 2.7 or even follow the 2.x releases over time.

Given that I wonder whether this issue should be critical. Yes its super annoying
that we use the old API, but it doesn't feel like a real critical.

Anyone wants to disagree?

Retest - just wanted to see what was what after #2389287: Missing PhpExecutableFinder

This should at least allow Drupal to install, albeit with user_validate_name disabled for now.

Or it might if I upload my new patch.

My changes, as I didn't get an interdiff:

1. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -239,9 +239,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    if ($error = user_validate_name($form_state->getValue(array('account', 'name')))) {
   -      $form_state->setErrorByName('account][name', $error);
   -    }
   

   I've commented this out as I couldn't work out how to fix it and it was preventing installation. Obviously we'll need a proper fix before committing.

2. +++ b/core/lib/Drupal/Core/TypedData/Validation/ClassMetadata.php
   @@ -0,0 +1,105 @@
   +    if ($this->typedData instanceof ListInterface) {
   +      return range(0, count($this->typedData) - 1);
   +    }
   +    elseif ($this->typedData instanceof ComplexDataInterface) {
   

   Both branches were looking for ComplexDataInterface

I'm making progress, I've commented the bits I'm currently stuck on with 'help wanted' in the patch. I've been using the edit user for to test for now.

I'm making progress, but there are two things I don't understand:
1) Why RecursiveContextualValidator is insisting getMetadataFor() returns a ClassMetadataInterface, when we sometimes want to return a PropertyMetadataInterface. I've asked for help on that at https://github.com/symfony/symfony/issues/11393
2) Why UserNameConstraintValidator is getting $items as an array, when it was written to expect an object (I assume TypedData\ListInterface).

I can get some validators to work by changing Metadata::getPropertyValue() to return $this->typedData instead of $this->typedData->getValue(), but doing so breaks others that are expecting other types (PrimitiveTypeConstraintValidator, Symfony's EmailValidator).

There's also a problem in EntityChangedConstraintValidator, because $this->context->getMetadata() returns a Symfony GenericMetadata, when our test needs a drupal Validation\Metadata.

Whether this is critical depends on the answer to #2395443: [policy, no patch] Follow symfony 2.7 or 3.0., this is the question. Unless we make a commitment to never use the 3.x branch of Symfony with 8.x, I think it probably should be though.

@catch : I might be missing something, but I don't get how this is tied to the decision about Symfony 3 ?
This issue is about using Validation 2.5.

Also, apparently it would let us solve #2023465: Allow validation constraints to add in message replacements

@yched The old, 2.4, API will not be supported in 3.x

@yched: It is related because that is when they will remove the old API/BC layer that we are currently using.

Adding #2400407: [meta] Ensure vendor (PHP) libraries are on latest stable release as a parent issue. This isn't strictly a vendor library upgrade but it is a blocker for future ones.

As an alternative consider removing the validator entirely. To call the DX bad is offensive to bad things. pwolanin tried to fix it and failed so I recommend admitting failure and postpone to D9. Reminder: the DX is putting a plugin id in ->setConstraints(array( and then putting the validation logic into a magic named class, the plugin class plus the Validator postfix. Due to the declarative nature of the definition it is almost impossible to figure this out.

@chx
To be fair, you would have to admit that you need to replace it with a system which works for the complex structure of entities/typed data
at the same time. The effort to rewrite it probably requires as much effort.

Symfony supports typed data...? *blink*

Edit: what I am trying to say, make the plugins standalone somehow. pwolanin tried to keep Symfony and move the validator class inside the plugin class and failed so I am recommending nuking Symfony and using our own logic to fire validation and thus keeping the plugins standalone. My favorite tune is this: the Drupal-Symfony integration is horrid and we need either fix it or where we can't fix it we should postpone adding Symfony.

diff --git a/core/lib/Drupal/Core/TypedData/TypedDataManager.php b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
old mode 100644
new mode 100755
index d42a3d2..143636d
--- a/core/lib/Drupal/Core/TypedData/TypedDataManager.php
+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -322,7 +322,7 @@ public function getValidator() {
       $this->validator = Validation::createValidatorBuilder()
         ->setMetadataFactory(new MetadataFactory())
         ->setTranslator(new DrupalTranslator())
-        ->setApiVersion(Validation::API_VERSION_2_4)
+        ->setApiVersion(Validation::API_VERSION_2_5)
         ->getValidator();
     }

Can we set it to Validation::API_VERSION_2_5_BC instead?
AFAIK, it won't change anything in the patch, but would still allow older validators to still be used.
That would increase our interoperability, because there's no way libraries will be switching exclusively to the new API (most still support symfony 2.3 because it's LTS, but even when 2.7 comes out there will be no pressure for them to move off the old API).

So, the argument here is interoperability with other libraries contrib authors might want to pull in.

@chx we already are manually triggering the validation, which makes this harder to debug because it's triggered in different places, for example TypedData::validate() and ComplexDataConstraintValidator::validate(). /@chx

Interestingly the latter call passes $constraints as a parameter to the symfony function RecursiveContextualValidator::validate(), which causes symfony to create a new symfony GenericMetadata instance instead of a drupal Metadata or ClassMetadata instance.

GenericMetadata has a more complicated CascadingStrategy and TraversalStrategy than our classes. It looks like this is how we should be stopping the excessive recursion I mentioned in my patch, but I wasn't able to get it working yet and it's late.

@ianthomas_uk
One of the hunks I made locally for experimental reasons is the following: This makes the unique validator a bit more generic.

I tried to get this working with Validation::API_VERSION_2_5 (once that works we can still opt to do Validation::API_VERSION_2_5-BC even), but I failed.

I ran into following problems:
Not a problem per se, but there is no generic property container metadata interface any more. Now, each object is automatically validated via validateObject() which assumes classmetadata. We do not want to validate any typical class properties, so this is not a good fit for us. Still, our objects are objects that need validation so that could be ok if it works out.
The main problem I ran into is that validator assumes that each class property is a "generic node", i.e. it does not detect properties that are objects - but that's critical for us to have. We cannot use implicit traversal as we need to preprocess the value validated as we can do it via getPropertyValue() (see below also).
While for class properties, we could massage the value independently of the metadata object (see MetadataBase::getPropertyValue()), we cannot do so for the initial object that's passed to the validator for validation. We should be work around this by preprocessing things before invoking symfony validator and passing a simple, scalar value and its contraints only if a non-container is to be validated - not nice, but that would probably work.

Maybe this can still be implemented using the cascading feature, I did not wrap my head around this yet but it from reading its docs it sounds like it shouldn't be necessary to use.

Udated (not working) patch attached.

Figured cascading is exactly what we need / I missed before. So we do explicit traversable and cascading, no implicit traversable. -> Got the tree traversal working that way.

Still one problem with the following left:

We should be work around this by preprocessing things before invoking symfony validator and passing a simple, scalar value and its contraints only if a non-container is to be validated - not nice, but that would probably work.

This has issues with the primitive type constraint type validator as it cannot access the typed data object for validation any more. We could solve this by splitting the constraint validation up into multiple constraints and adding in the right constraint upfront.

Updated patch attached, not fully working yet.

Attaching right patch file, interdiff was correct.

While debugging this it seemed to be that the validation got applied to two levels, not sure whether its FieldItemList and FIeldItem or FieldItem and the typed data.

... Things broke a bit more in the meantime.

It seems to be that there is no way to get back to the typed data inside the ConstraintValidation ... one way around it seems to be that explicitly allowing to set the typed data on the constraint and access that via the Validation ... It is really odd that we are now not longer possible to set our own metadata.

See some experiments in the interdiff.

Didn't I just post here? Thanks d.o. for eating my (rather long) comment :/

I did discuss this a little with webmozart while talking about #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay. Traversable and cascading are in general both fine, but traversable does not allow us to process the property value before validation. So we need to use cascading and turn implicit traversing off in order to avoid double-validation. That way we can leverage getPropertyValue() of the class metadata to process the values before validation + keep the extra-check for primitives when invoking validation. So far this seems like it should work.

Generally, we'll have to use ClassMetadata for all containers, and can use some property metadata for the others. So containers are "classes" and non-containers "properties" for the validator.

It seems to be that there is no way to get back to the typed data inside the ConstraintValidation ... one way around it seems to be that explicitly allowing to set the typed data on the constraint and access that via the Validation ... It is really odd that we are now not longer possible to set our own metadata.

Yeah, I was thinking about that as well. At least a helper method getTypedData() would probably be helpful.
#77 looks quite ugly though: Shouldn't it still work with $this->context->getObject()? From the docs it looks like it should give us the next parent object?

planning to work on this

looks like this patch may conflict with #2346373: Data reference validation constraints are applied wrong
should one or the other be postponed blocked on the other?

@sidharrell, no we don't postpone on conflicts - but thanks for pointing it out.

I'm slowly making progress on this, will continue tonight after work.

interdiff against #72
We're already passed $value, which is typed-data, not sure why we had to try and re-get it from context.
This creates duplicate messages, so will fail, but at least we're getting somewhere.
Un-assigning in case others want a look, but will prod some more tomorrow or perhaps later tonight.

Also, injected the current user whilst at-it

So $value isn't always typed data

Currently working on it, but I'm convinced that we need the following patch.
Now I'm looking at the double validation issue.

This passes FeedValidationTest locally

merged HEAD

interdiff at #94 is against #89

installs locally√

Still getting fails on other validations locally, but progress

this starting to get on the mental side, wrapping a property in an envelope with the constraints we want and the typed data we need.

I think we need to rethink how ComplexDataConstraintValidator works, replacing it with property metadata.

This is so funny, I had the exact same approach but I'm convinced that the interdiff in #93 is all we actually need.

Created an issue in symfony: https://github.com/symfony/symfony/issues/13885

Actually, after sleeping on this - I think we can make PropertyContainerMetadata smart enough to recognise ComplexDataConstraint and serve the properties/constraints using getPropertyValue|getConstrainedProperties|hasPropertyMetadata|getPropertyMetadata i.e. we recognize ComplexDataConstraintValidator in PropertyContainerMetadata::{find|get}Constraints and register the properties/constraints internally in PropertyContainerMetadata and then remove ComplexDataConstraint from the list of constraints, and allow the RecursiveContextualValidator to validate them. When the RecursiveContextualValidator asks PropertyContainerMetadata for properties to validate (getConstrainedProperties) we can return the properties that were contained inside the ComplexDataConstraint constraint, then we can use hasPropertyMetadata|getPropertyMetadata to return the property constraints and getPropertyValue to handle the 'fetch the property value' logic that was in ComplexDataConstraintValidator. We can then remove ComplexDataConstraintValidator.

Implements idea in #105
Still needs work for traversal/cascade because we're getting duplicates - and some constraints no longer working, but I think step in the right direction - more tomorrow.

Just providing information

Symfony 2.6.5 was released today

http://symfony.com/blog/symfony-2-6-5-released?utm_source=feedburner&utm...

#2105797-41: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered suggests this is blocking it

@martin107: Thanks, I created another issue for the Symfony update: #2454393: Upgrade to Symfony 2.6.5.

More work handling typed data correctly now, but still missing the secret sauce on travesal/cascade.

If we say 'cascade' on the entity, some tests pass, others don't.

Looking for some help here.

full interdiff

a broken patch is better than no patch

more tomorrow

More work handling typed data correctly now, but still missing the secret sauce on travesal/cascade.

See #78 in case you missed that.

It might help simplifying things a bit to customize the NotNull constraint to support typed data and check isEmpty() for complex data and lists. Then, we can save the headache of massing empty typed data to NULL values.

Spend an evening trying to understand what is going on.

One problem I encountered is that we stop the cascading as we don't take into account the PrimitiveItem based constrains, when we determine the cascading.
I solved that by making the cascading determination recursive.

On top of that we miss to pass along the constraints ... sadly now we have double constraints again ...

This is the code which is executed for the two code paths which lead to the duplicate path:

First one:

We validate the entity object validateClassNode
-- we validate each property of the object 572: $metadata->getConstrainedProperties() => validateGenericNode // These are the item lists
validateGenericNode:
-- line 702 => validateInGroup $value being the $itemList
-- line 759 => validateObject: cascading for the item list
-- validateClassNode
-- line 562: => validateInGroup

So once we validate the item list as part of being a property of the entity and once by being an object for itself.

This though seemed to be take into account by the architecture by the following code in validateInGroup:

    private function validateInGroup($value, $cacheKey, MetadataInterface $metadata, $group, ExecutionContextInterface $context)
    {
        $context->setGroup($group);

        foreach ($metadata->findConstraints($group) as $constraint) {
            // Prevent duplicate validation of constraints, in the case
            // that constraints belong to multiple validated groups
            if (null !== $cacheKey) {
                $constraintHash = spl_object_hash($constraint);

                if ($context->isConstraintValidated($cacheKey, $constraintHash)) {
                    continue;
                }

It caches the constraint calls and tries to not execute it multiple times.

... Let's have a look what $cacheKey is. For the first call its 8b50000000139a060ad:title, for the second call its 8810000000139a060ad.
Two different cache paths, once for the validation of the property of the entity, once, for the item list as its own object ...

With the following patch, we can at least get the same constraints object, by caching the metadata, and so the constraints by typed data object.

First call:
cacheKey:       8b50000000139a060ad:title
constraintHash: fe90000000139a060ad
$items:         8810000000139a060ad

Second call:
cacheKey:       8810000000139a060ad
constraintHash: fe90000000139a060ad
$items:         8810000000139a060ad

Sent webmozart a mail, to see whether he has some clue about what is going on here.

Since this is blocking #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered, tagging as "Entity validation"

https://github.com/dawehner/symfony/tree/test-nested-objects is some first test, not yet done, to try to reproduce the bit mentioned in #122
on upstream code.

It turns out, rerolling after #2346373: Data reference validation constraints are applied wrong is a bit of work.

Not done yet, but I've got cascading working without duplications. What's left is fixing the complex constraint validators. I think adding a trait for getTypedData() should work here.

So turns out the main problem left is allow validators to fetch the typed data object. Turns out, this seems impossible to do with symfony validator 2.5 API is there is no way to process the initial object passed to the validator before it's passed on to validators.

I tried doing an intermediate workaround as discussed with dawehner, but turns out that does not work out either. The workaround works with a custom constraint to wrap things, but as soon as that is used the generic metadata is used and cascading is skipped. That makes entity validation fail. For completeness, attached is the version of the patch with this failing approach.

So looks like the only option left to ensure getTypedData() continues to work is improving symfony validator. I'll can try that later this week, but unassigning for now.

Working on this.

ok, so here is a patch taking the new approach of fixing things in symfony validator instead of hacking around it. Let's see how this goes. Changes to symfony validator need to be contributed to validator and added in via composer still.

This actually fixes a regression in validator which removes property paths with a leading zero ("0.value") and improves it to allow value preprocessing.

Added API changes:

• Type hints to the 2.4 interface \Symfony\Component\Validator\ValidatorInterface must be updated to use the 2.5 interface \Symfony\Component\Validator\Validator\ValidatorInterface
• Constraints on fields can be called with empty item objects now, instead of being called with NULL.

This should fix test fails - which are due to above API changes.

This is very nice work. Once the patch is green can we create PR against symfony/Validator and add a todo here?

+++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataAwareValidatorTrait.php
@@ -0,0 +1,36 @@
\ No newline at end of file

+++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadataInterface.php
@@ -0,0 +1,39 @@
\ No newline at end of file

nit

Not addressed #140 yet.

Green!!!!!
Can you add a diff for vendor only? After spending many an evening with this patch and in the recursive validator, keen to learn what the secret sauce was.
fago+=1000

+++ b/core/vendor/symfony/validator/Symfony/Component/Validator/Mapping/PreprocessingMetadataInterface.php
@@ -0,0 +1,35 @@
+ * they are passed on the constraint validators.

%s/on/onto ?

The changes to the symfony component look fairly non-controversial to me, great work @fago

fixes 140 and some other minor nits

thanks. I've posted PRs for our changes: https://github.com/symfony/symfony/pulls/fago and opened tickets:
https://github.com/symfony/symfony/issues/14394
https://github.com/symfony/symfony/issues/14396

I'll reroll the patch to use https://github.com/fago/validator for now.

I had a look at the recent changes and yeah they are great!

worked on adding in a interim validator of mine that adds the PRs. It exposes some remaining issues with old API usage.

Unassigning while not working on it.

@fago
What are the next steps here, do we have to wait on the upstream PRs or can someone help with those?

I'll discuss them with webmozart today, to make sure we are on the same page of the upstream issues. As soon as that's the case I think we could move on here. If there is agreement on the general direction of upstream PRs I think we could even move on while they are still being worked on. But if things move fast anyway, it would be better to wait for them of course.

I think it depends how much of a blocker this is on other work, and how quickly we think the upstream issues will be resolved. In the past when we needed fixes to e.g. Doctrine we temporarily pointed composer.json to a fork to get around this problem. However, given we're in beta and there are some actual sites on D8 at this stage, I'd prefer not to do that unless absolutely necessary.

@webchick: Thanks, makes sense.

Unfortunately, webmozart cannot make it today, so we rescheduled discussion for tomorrow afternoon instead. I'll report back afterwards.

Discussed with webmozart via skype/hangouts.

He considers https://github.com/symfony/symfony/pull/14395 as a simple bug fix (as we do), so this should not be an issue to solve. For https://github.com/symfony/symfony/issues/14396, we've been wondering whether it doesn't make sense to throw away Validator's traversal logic and replace at with our own as a whole - as I already discussed with dawehner. This would give us more flexibility and save us from needlessly sticking with Symfony validator's class based metadata concept what does not fit so well for our approach. We'll meet again in person tomorrow to explore this option further.

we've been wondering whether it doesn't make sense to throw away Validator's traversal logic and replace at with our own as a whole

Is this something that can be quickly prototyped in order to see what it might look like before deciding?

I met with webmozart today and we discussed more details based upon which I started implementing this. It's WIP and considered prototype-code, but it should be pretty complete. It's completely untested, so making it actually work would be the next step.

I really like how this looks compared to the previous approach, as the code and required logic WAY simpler. Instead of mapping our structure to symfony metadata's structure we can just stick with our structure and benefit from our existing metadata and use a simple traversal logic. However, being able to do so requires taking over some (rather simple) classes to avoid API compat. problems with internal classes (e.g. ValidationConstraintBuilder).

The implemenation does not cover groups at all - as we do not need them and did not implement them before in our Metadata implementations either. This allows our RecurisveValidator to save quite some code over the symfony one. If we decide we want to add this feature later on, we'd have to add a respective implementation also.

Of course, our recursive validator implementation would need test coverage also.

Let's update the IS after #155

I really like how we have now code which is actually comprehensible and followable, that itself is a huge win!

1. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
   @@ -0,0 +1,164 @@
   +class RecursiveContextualValidator implements ContextualValidatorInterface {
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function validateNode(TypedDataInterface $data, $constraints = NULL) {
   

   I love that kind of documentation :P

2. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
   @@ -0,0 +1,164 @@
   +      throw new \LogicException('Passing custom groups is not supported.');
   

   As probably said personally, I think not implementing groups at the moment is totally fine. Its a feature which we don't use really and could add later, if it really would be needed.

3. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
   @@ -0,0 +1,164 @@
   +    // Validate properties, if any.
   +    if ($data instanceof TraversableTypedDataInterface) {
   

   It would be great to document that this not only iterates over properties but also items of a FieldItemList/ListInterface

4. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
   @@ -0,0 +1,164 @@
   +      // Prevent duplicate validation of constraints, in the case
   +      // that constraints belong to multiple validated groups
   +      if (null !== $cacheKey) {
   +        $constraintHash = spl_object_hash($constraint);
   +
   +        if ($this->context->isConstraintValidated($cacheKey, $constraintHash)) {
   +          continue;
   +        }
   +
   

   I'm curious whether we still need that projection now that we have a way simpler system? Does it still happen that we validate the system thing multiple times?

5. +++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php
   @@ -0,0 +1,81 @@
   +    return TraversalStrategy::NONE;
   ...
   +    // By default, never cascade into validating referenced data structures.
   +    return CascadingStrategy::NONE;
   

   If I understand correctly, we no longer use those methods so should we just throw an exception here as well?

6. +++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadataInterface.php
   @@ -0,0 +1,28 @@
   + *
   + * As each typed data object can serve as validation root, the metadata has to
   + * implement ClassMetadataInterface.
   

   it seems to be that this statement is no longer valid?

7. +++ b/core/lib/Drupal/Core/Validation/ConstraintManager.php
   index efc6906..ef1fce8 100644
   --- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
   
   --- a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
   +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/AllowedValuesConstraintValidator.php
   

   I guess we could reduce the amount of changes here, all we need is the new trait, as far as I understand.

Working on some basic unit test for some of the new classes now.

Removed unsupported method setMetaDataFactory from /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/TypedData/TypedDataManager.php on line 335

@dashaforbes
Please always post an interdiff, otherwise its hard to check what you changed.

Here is a patch which ignores the changes in #161 so far, but adds the promised unit test with some "minor" fixes, just to get the unit tests running.

Applies 161 to 162

ok, worked over it to make it actually working. This should be pretty complete, only some docs need love and tests need to be improved of course. I made the new test basically working by make use of real type data objects - I do not think we should mock them but test it with real Typed Data objects. That works, I did not convert one test method though.

It would be great to document that this not only iterates over properties but also items of a FieldItemList/ListInterface

This works on typed data level, not entities and fields - thus we should document this using typed data terminology. Clarified that though.

I'm curious whether we still need that projection now that we have a way simpler system? Does it still happen that we validate the system thing multiple times?

The methods for it are in the context interface. I do not think it's really needed, but it won't hurt either.

If I understand correctly, we no longer use those methods so should we just throw an exception here as well?

I think it's fine to return the value - if someone would ask this metadata class it just says no thanks. No reason to throw an exception imho.

it seems to be that this statement is no longer valid?

I removed the interface in favor of $this->context->getObject() - I just made the object of our value to be always the Type Data object what seems reasonable to me.

I guess we could reduce the amount of changes here, all we need is the new trait, as far as I understand.

Yes, those changes were introduced somewhere above and I just kept it as they make sense. I'd be fine with moving those to another issue though.

Fixed two problems:

• The default constraint group was wrongly marked as validated even when custom $constraints where passed in - fixed that. Only when metadata constraints are validated it can be considered as "default" validation.
• Unlike symfony validator and our previous implementation, we were continuing validation for empty structures (e.g. an empty field). However, empty fields won't be saved anyway and empty items will be filtered out, thus I think it's safe to keep the previous and default practice of not validating empty structures. Also, this saves us from double violation messages due to empty fields and properties.

Thanks @dawehner

I'm trying to fix the few remaining test failures this morning.

The remaining problems are all around the problem of constructing the right property path.

We have two problematic cases:

1. We want to validate an item list for itself. This happens for example in core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php:230
   For that case, we want to start with an empty property path for the root level. So $item_list['0']['property'] should have 0.property as path.
2. We want to validate complex data, see \Drupal\Core\Validation\Plugin\Validation\Constraint\ComplexDataConstraintValidator. In that case we want to continue the current property path.

The easiest solution for that problem is to introduce an option $root_call variable on the validate() method. I played also around with atPath(),
but for now I sticked with $root_call (more experiments later).

Some additional information about property paths: Property paths generally differ between array access and property access:

• 0.property corresponds to the PHP code $obj->0->property or $obj->get0()->getProperty()
• [0][property] corresponds to the PHP code $obj['0']['property']

You can mix and match the two, e.g.

• [0].property corresponds to the PHP code $obj['0']->property or $obj['0']->getProperty()

I recommend you to construct the property paths accordingly. If you don't, you might run into problems later that we ran into with Symfony:

• If some code does make a difference between array and property access, it's impossible to reverse-map the path 0.property.
• You'll be able to integrate the symfony/property-access component which simplifies handling of property paths, should you need it in the future.

If you don't follow the conventions now, you block the door for the above points (and maybe more) without breaking BC in future releases.

@dawehner: thanks

@webmozart: Thanks, I do not think it is a problem though as we have all metadata in place to do the traversal depending whether we deal with lists or complex data anyway - besides that it's always just a get() call for us. So we have no reason to put it into our property paths, what keeps applying them and mapping them to form elements simpler.

Here is the interdiff, sorry for forgetting to upload it.

   /**
    * {@inheritdoc}
    */
-  public function isGroupValidated($cacheKey, $groupHash) {
-    return isset($this->validatedObjects[$cacheKey][$groupHash]);
+  public function isGroupValidated($cache_key, $group_hash) {

Are you sure you want to rename parameters declared by a parent interface, and documented there? It immediately makes the docblock wrong.
Core has generally elected to leave camelCase for code that extends Symfony.

(Yes, it's inconsistent, but that's the core coding standard we need to change after D8 is released).

Fixed the remaining failure, not sure whether this is the perfect fix though.

Just doing some basic review here.

1. +++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContext.php
   @@ -0,0 +1,317 @@
   +   * @var ValidatorInterface
   ...
   +   * @var TranslatorInterface
   ...
   +   * @var ConstraintViolationList
   ...
   +   * @var MetadataInterface|null
   ...
   +   * @param mixed $root The root value of the
   +   *                                               validated object graph
   +   * @param TranslatorInterface $translator The translator
   +   * @param string|null $translationDomain The translation domain to
   +   *                                               use for translating
   +   *                                               violation messages
   +   *
   

   Let's use FQCN

2. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
   @@ -0,0 +1,210 @@
   +  protected function validateNode(TypedDataInterface $data, $constraints = NULL, $root_call = FALSE) {
   

   @fago
   So are you fine with adding the $root_call change?

3. +++ b/core/tests/Drupal/Tests/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidatorTest.php
   @@ -24,17 +24,10 @@ class PrimitiveTypeConstraintValidatorTest extends UnitTestCase {
   diff --git a/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php b/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   
   diff --git a/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php b/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   index 191decd..6f82d6d 100644
   
   index 191decd..6f82d6d 100644
   --- a/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   
   --- a/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   +++ b/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   
   +++ b/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   +++ b/core/vendor/symfony/validator/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php
   @@ -92,7 +92,7 @@ public function atPath($path)
   
   @@ -92,7 +92,7 @@ public function atPath($path)
        /**
   

   That change is certainly out of scope :)

Fixing a couple of those nitpicks.

Thanks, great to see it green again.

+++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataAwareValidatorTrait.php
@@ -27,7 +28,7 @@ public function getTypedData() {
-    if (!$data instanceof TypedDataInterface) {
+    if (!$data instanceof TypedDataInterface && !$data instanceof PrimitiveInterface) {

Why that? Was that necessary somehow? That does not make sense to me.

Are you sure you want to rename parameters declared by a parent interface, and documented there? It immediately makes the docblock wrong.Core has generally elected to leave camelCase for code that extends Symfony.

I hated doing so but didn't know better. If we don't have to do that, perfect.

That change was needed for

 class PrimitiveTypeConstraintValidator extends ConstraintValidator {
  $typed_data = $this->getTypedData();

$typed_data here is primitive most of the time, see all the checks below in that file.

Still, our primitives are Typed data - so was the problem only the test? Probably the interface should extend TypedDataInterface even.

Ah yeah the problem was the test.

Worked on a basic change record, feel free to add more stuff to it / correct stuff.
Also updated the issue summary.

+++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContext.php
@@ -0,0 +1,317 @@
+   * @param TranslatorInterface $translator The translator
+   * @param string|null $translationDomain The translation domain to
+   *                                               use for translating
+   *                                               violation messages

+++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContextFactory.php
@@ -0,0 +1,50 @@
+   * @param TranslatorInterface $translator The translator
+   * @param string|null $translationDomain The translation domain to
+   *                                               use for translating
+   *                                               violation messages

Is this intentional?

Is this intentional?

Well, the problem is that the classes are coming directly from symfony, but sadly, for some reason, they are marked as internal, so we can't safely use them.
Let's keep them in sync in order to have an easy life to sync those classes in the future.

1. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
   @@ -100,10 +100,12 @@ public function validate($data, $constraints = NULL, $groups = NULL) {
   +   * @param bool $root_call
   +   *   (optional) Whether its the most upper call in the type data tree.
   

   What about calling it $is_root ? And documentating it as "Whether the node is the root of the validation tree; i.e., the node where validation started."

2. +++ b/core/tests/Drupal/Tests/Core/TypedData/RecursiveContextualValidatorTest.php
   @@ -152,54 +153,46 @@ public function testBasicValidateWithMultipleConstraints() {
   -    $tree = ['value' => []];
   +    $tree = ['value' => ['key1' => 'value1', 'key2' => 'value2', 'key_with_properties' => ['subkey1' => 'subvalue1', 'subkey2' => 'subvalue2']]];
   

   minor: Could be formatted nicer.

Also, left is the following todo:

  public function atPath($path) {
    // @todo This value is not used at the moment.

So we need to either implement and test this, or just throw an unsupported exception. I'm not sure what would be th use of this, maybe webmozart can clarify?

Then I think we miss test-coverage for:
- validateProperty()
- validatePropertyValue()

Finally, I'm wondering about the two left Symfony-ish construct() comments. As that code is rather trivial, I don'T think we'll have to update it with a future Symfony version instead of just improving it. Thus, I think we can just drupalize those two left comments also.

I went through the patch and fixed a few doc nits. Besides that and the above I think this is complete and so almost ready.

Also added the following overview to the summary:

Symfony Validator API usage

+ ConstraintViolationBuilderInterface
+ ExecutionContextFactoryInterface
+ ContextualValidatorInterface
+ ConstraintViolation
+ ConstraintViolationList
+ PropertyPath

Classes, of which we created our own variant because they are internal in Symfony:
ConstraintViolationBuilder
ExecutionContextFactory

According to the Symfony BC promise this non-internal usages should be fine. Worst thing that can happen is a added method to one of the interfaces, which would be noted in the uprade notes.

@fago
So are you fine with adding the $root_call change?

In general, yes I think the solution is fine. Just see above about a docu improvement suggestion.

What about calling it $is_root ? And documentating it as "Whether the node is the root of the validation tree; i.e., the node where validation started."

What about using $is_root_call? \Drupal\Core\Render\RendererInterface::render is using the same name.

Finally, I'm wondering about the two left Symfony-ish construct() comments. As that code is rather trivial, I don'T think we'll have to update it with a future Symfony version instead of just improving it. Thus, I think we can just drupalize those two left comments also.

Sure why not.

What about using $is_root_call? \Drupal\Core\Render\RendererInterface::render is using the same name.

I see, fine to me!

• As we have extended the method, should we not extend the interface, just to be safe? Did that.
• Expanded the test coverage for validateProperty() and validatePropertyValue() and fixed the bug, I think.

Now I think this patch should be ready, right before comment #200

Forgot the interface file.

1. +++ b/core/lib/Drupal/Core/TypedData/Validation/ConstraintViolationBuilder.php
   @@ -0,0 +1,220 @@
   +      try {
   +        $translatedMessage = $this->translator->transChoice($this->message, $this->plural, $this->parameters, $this->translationDomain#
   +        );
   

   why is there a hash # at the end of line?

2. +++ b/core/lib/Drupal/Core/TypedData/Validation/ContextualValidatorInterface.php
   @@ -0,0 +1,36 @@
   +   * @param bool $is_root_call
   +   *   (optional) Whether its the most upper call in the type data tree.
   

   should be "typed data" here, right?

3. +++ b/core/lib/Drupal/Core/TypedData/Validation/PropertyPath.php
   @@ -0,0 +1,35 @@
   +      return $basePath !== (string) '' ? $basePath . '.' . $subPath : $subPath;
   

   why are we casting the empty string '' to (string) here, does no make sense? Did you want to cast $basePath instead?

Thank you for your review klausi!!

why are we casting the empty string '' to (string) here, does no make sense? Did you want to cast $basePath instead?

Really good point!