Problem/Motivation

Might be blocking #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered.

#2278353: Update to Symfony 2.5 is going with the 2.4 Symfony validation integration API. Because HEAD is now on Symfony 2.5, we should upgrade the validation API integration.

Proposed resolution

Use the 2.5 API validation.

We are achieving this by implementing the validator interfaces in our own code. This makes the code much easier to follow for the case
of validating an entity / typed_data and also works around problems in implementing the newer metadata API.

Remaining tasks

None

User interface changes

None.

API changes

  • All validation calls must use the 2.5 style.
  • 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.

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.

Files: 
CommentFileSizeAuthor
#216 validator-2.5-2343035-5.215.patch106.67 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,218 pass(es). View
#215 upgrade_validator-2343035-215.core-only.do-not-test.patch83.11 KBlarowlan
#215 interdiff.txt1009 byteslarowlan
#214 2343035-214.patch106.66 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,212 pass(es). View
#214 interdifff-with-update.txt33.71 KBdawehner
#214 interdiff.txt4.79 KBdawehner
2343035-211.patch79.25 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,191 pass(es). View
#209 interdiff.txt14.72 KBdawehner
#209 2343035-207.patch79.18 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,186 pass(es). View
#204 d8-upgrade_validator-2343035-204.patch81.59 KBjibran
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,944 pass(es). View
#204 interdiff.txt3.29 KBjibran

Comments

cilefen’s picture

Status: Active » Postponed
hussainweb’s picture

Status: Postponed » Active

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.

webmozart’s picture

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’s picture

Issue summary: View changes
cilefen’s picture

@webmozart: If Drupal 8 does not ever go to SF 3, then we need not do anything. I have heard the plan is to get to 2.7 and stay there for a while.

webmozart’s picture

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

cilefen’s picture

@webmozart So we may want to proceed with this issue. The goal would be to change the API level, which is set in TypedDataManager, and upgrade all the validation calls.

cilefen’s picture

@webmozart So we may want to proceed with this issue. The goal would be to change the API level, which is set in TypedDataManager, and upgrade all the validation calls.

cilefen’s picture

Status: Active » Needs review
FileSize
655 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

This is merely the API version change. I expect there will be test failures.

cilefen’s picture

@webmozart Is it the case that if we do not call setApiVersion() then the API decision is automatic? It seems obvious.

Status: Needs review » Needs work

The last submitted patch, 9: upgrade_validator-2343035-9.patch, failed testing.

webmozart’s picture

@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’s picture

@webmozart Thank you for the explanation. Drupal 8 requires 5.4+. Considering that, this a choice between doing nothing and remaining on 2.5-BC or going to 2.5. In your opinion, is that a good idea in terms of new features?

webmozart’s picture

@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.

cilefen’s picture

Status: Needs work » Needs review
FileSize
658 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

This is with the 2.5 backwards-compatible API.

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 15: upgrade_validator-2343035-15.patch, failed testing.

Miroling’s picture

Assigned: Unassigned » Miroling
Miroling’s picture

FileSize
2.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,751 pass(es). View
Miroling’s picture

FileSize
2.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,929 pass(es), 65 fail(s), and 6,750 exception(s). View
Miroling’s picture

Status: Needs work » Needs review

Need automatic patch testing

Status: Needs review » Needs work

The last submitted patch, 21: upgrade_validator-2343035-17.patch, failed testing.

Miroling’s picture

Assigned: Miroling » Unassigned

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’s picture

@Miroling: Thanks for working on this issue. Some tips: name the patches after the comment number. https://dreditor.org/ helps you find the next patch filename. Interdiffs are appreciated https://www.drupal.org/documentation/git/interdiff.

Miroling’s picture

@cilefen Ok, Thank you for suggestions :)

webmozart’s picture

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

cilefen’s picture

@webmozart: Thank you for following up on this issue. I am asking around on IRC whether this change is too disruptive now that D8 is in beta.

catch’s picture

Priority: Normal » Critical

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.

cilefen’s picture

Issue summary: View changes

@webmozart: The task is to get the tests passing with the patch in #21. Now that this issue is critical it will receive more attention.

cilefen’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,549 pass(es), 60 fail(s), and 6,859 exception(s). View
5.47 KB

Drupal\Core\TypedData\Validation\Metadata uses the deprecated Symfony\Component\Validator\PropertyMetadataInterface also.

Status: Needs review » Needs work

The last submitted patch, 31: upgrade_validator-2343035-31.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
6.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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.

webmozart’s picture

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

Status: Needs review » Needs work

The last submitted patch, 33: upgrade_validator-2343035-33.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
545 bytes
6.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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.

hussainweb’s picture

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

Status: Needs review » Needs work

The last submitted patch, 36: upgrade_validator-2343035-36.patch, failed testing.

dawehner’s picture

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?

cilefen’s picture

dawehner I agree with you.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: upgrade_validator-2343035-36.patch, failed testing.

martin107’s picture

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

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
6.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

ianthomas_uk’s picture

FileSize
8.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,087 pass(es), 1,471 fail(s), and 296 exception(s). View

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

The last submitted patch, 44: upgrade_validator-2343035-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 45: upgrade_validator-2343035-44.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
10.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,326 pass(es), 984 fail(s), and 267 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 48: upgrade_validator-2343035-48.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
10.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,845 pass(es), 85 fail(s), and 43 exception(s). View

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).

Status: Needs review » Needs work

The last submitted patch, 50: upgrade_validator-2343035-50.patch, failed testing.

ianthomas_uk’s picture

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.

catch’s picture

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.

yched’s picture

@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

ianthomas_uk’s picture

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

Berdir’s picture

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

catch’s picture

Issue tags: +Triaged D8 critical
catch’s picture

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.

catch’s picture

chx’s picture

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.

dawehner’s picture

@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.

chx’s picture

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.

bojanz’s picture

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.

ianthomas_uk’s picture

FileSize
2.47 KB

@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.

dawehner’s picture

FileSize
1.13 KB

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

fago’s picture

Status: Needs work » Needs review
FileSize
13.69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
18.52 KB

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.

Status: Needs review » Needs work

The last submitted patch, 66: d8_validator.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_validator_0.patch. Unable to apply patch. See the log in the details link for more information. View
4.29 KB

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.

fago’s picture

FileSize
15.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,274 pass(es), 117 fail(s), and 15 exception(s). View

Attaching right patch file, interdiff was correct.

The last submitted patch, 68: d8_validator.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 69: d8_validator.patch, failed testing.

cilefen’s picture

FileSize
15.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,556 pass(es), 121 fail(s), and 12 exception(s). View

Reroll.

andyceo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 72: upgrade_validator-2343035-72.patch, failed testing.

dawehner’s picture

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.

dawehner’s picture

... 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.

dawehner’s picture

FileSize
4.32 KB

See some experiments in the interdiff.

fago’s picture

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?

larowlan’s picture

Assigned: Unassigned » larowlan

planning to work on this

cilefen’s picture

To those just getting oriented on this issue, here is where the different validator is selected:

        // Symfony\Component\Validator\ValidatorBuilder
        if (Validation::API_VERSION_2_4 === $apiVersion) {
            return new ValidatorV24($metadataFactory, $validatorFactory, $translator, $this->translationDomain, $this->initializers);
        }

        if (Validation::API_VERSION_2_5 === $apiVersion) {
            $contextFactory = new ExecutionContextFactory($translator, $this->translationDomain);

            return new RecursiveValidator($contextFactory, $metadataFactory, $validatorFactory, $this->initializers);
        }

The validate() functions in each one take different arguments.

cilefen’s picture

beejeebus’s picture

looking at this during DrupalSouth 2015.

current status - using the patch at 72 and the interdiff at 77, i get install failures during install.

Symfony\Component\Validator\Exception\InvalidOptionsException: The option "typedData" does not exist in constraint Drupal\user\Plugin\Validation\Constraint\UserNameConstraint in Symfony\Component\Validator\Constraint->__set() (line 195 of core/vendor/symfony/validator/Symfony/Component/Validator/Constraint.php).
Symfony\Component\Validator\Constraint->__set('typedData', Object)
Drupal\Core\TypedData\TypedData->getConstraints()
Drupal\Core\Field\FieldItemList->getConstraints()
Drupal\Core\TypedData\Validation\MetadataBase->findConstraints('Default')
Symfony\Component\Validator\Validator\RecursiveContextualValidator->validateInGroup(Object, '000000007890501e00007f2df4327681', Object, 'Default', Object)
Symfony\Component\Validator\Validator\RecursiveContextualValidator->validateClassNode(Object, '000000007890501e00007f2df4327681', 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()
user_validate_name('admin')
Drupal\Core\Installer\Form\SiteConfigureForm->validateForm(Array, Object)
call_user_func_array(Array, Array)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'install_configure_form')
Drupal\Core\Form\FormValidator->validateForm('install_configure_form', Array, Object)
Drupal\Core\Form\FormBuilder->processForm('install_configure_form', Array, Object)
Drupal\Core\Form\FormBuilder->buildForm('Drupal\Core\Installer\Form\SiteConfigureForm', Object)
install_get_form('Drupal\Core\Installer\Form\SiteConfigureForm', Array)
install_run_task(Array, Array)
install_run_tasks(Array)
install_drupal(Object)
beejeebus’s picture

Status: Needs work » Needs review
FileSize
19.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,279 pass(es), 799 fail(s), and 205 exception(s). View

just want to see how this runs, got install working again with the help of @larowlan.

Status: Needs review » Needs work

The last submitted patch, 83: 2343035-83.patch, failed testing.

beejeebus’s picture

not sure how to proceed on this.

fatals in the test are due to expectations in the new SF2 validators that the current code doesn't satisfy.

so, we're going to need to wrap SF2, or rewrite a bunch of code in the validators.

Drupal\Core\Validation\Constraint\AllowedValuesConstraint, for example, needs to have a public $choices or $callback in the new validator world. (it's kinda crappy that public variables are required, that's not much of an API.)

sidharrell’s picture

Issue summary: View changes
sidharrell’s picture

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?

larowlan’s picture

@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.

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
2.89 KB
18.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,426 pass(es), 668 fail(s), and 89 exception(s). View

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.

larowlan’s picture

Also, injected the current user whilst at-it

Status: Needs review » Needs work

The last submitted patch, 89: validator-2.5-2343035-2.89.patch, failed testing.

larowlan’s picture

So $value isn't always typed data

dawehner’s picture

FileSize
3.15 KB

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
18.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,694 pass(es), 100 fail(s), and 11 exception(s). View

This passes FeedValidationTest locally

larowlan’s picture

FileSize
18.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,676 pass(es), 106 fail(s), and 11 exception(s). View

merged HEAD

larowlan’s picture

interdiff at #94 is against #89

larowlan’s picture

installs locally√

larowlan’s picture

Still getting fails on other validations locally, but progress

Status: Needs review » Needs work

The last submitted patch, 95: validator-2.5-2343035-2.93.patch, failed testing.

The last submitted patch, 94: validator-2.5-2343035-2.93.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
8.64 KB
23.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,405 pass(es), 108 fail(s), and 30 exception(s). View

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.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 101: validator-2.5-2343035-2.95.patch, failed testing.

dawehner’s picture

larowlan’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
12.57 KB
20.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,261 pass(es), 650 fail(s), and 102 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 106: validator-2.5-2343035-2.106.patch, failed testing.

martin107’s picture

Just providing information

Symfony 2.6.5 was released today

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

hussainweb’s picture

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
6.62 KB
23.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,466 pass(es), 62 fail(s), and 24 exception(s). View

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.

larowlan’s picture

FileSize
6.76 KB

full interdiff

Status: Needs review » Needs work

The last submitted patch, 111: validator-2.5-2343035-2.111.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
22.84 KB

a broken patch is better than no patch

more tomorrow

Status: Needs review » Needs work

The last submitted patch, 114: validator-2.5-2343035-2.114.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 114: validator-2.5-2343035-2.114.patch, failed testing.

fago’s picture

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.

cilefen’s picture

TypedData class name changes: https://www.drupal.org/node/2457329

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
24.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,210 pass(es), 619 fail(s), and 90 exception(s). View

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 ...

Status: Needs review » Needs work

The last submitted patch, 120: 2343035-120.patch, failed testing.

dawehner’s picture

FileSize
24.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2343035-122.patch. Unable to apply patch. See the log in the details link for more information. View
2.81 KB

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
dawehner’s picture

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

effulgentsia’s picture

dawehner’s picture

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.

dawehner’s picture

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

fago’s picture

Assigned: Unassigned » fago
xjm’s picture

Issue tags: +D8 Accelerate Dev Days
fago’s picture

Status: Needs work » Needs review
FileSize
22.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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.

Status: Needs review » Needs work

The last submitted patch, 129: d8_validator.patch, failed testing.

fago’s picture

Assigned: fago » Unassigned
Status: Needs work » Needs review
FileSize
31.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 131: d8_validator.patch, failed testing.

fago’s picture

Assigned: Unassigned » fago

Working on this.

fago’s picture

Status: Needs work » Needs review
FileSize
38.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,264 pass(es), 40 fail(s), and 9 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 134: d8_entity_validate_general.patch, failed testing.

fago’s picture

Issue summary: View changes

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.
cilefen’s picture

Issue tags: +Needs change record
fago’s picture

Status: Needs work » Needs review
FileSize
44.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,356 pass(es), 40 fail(s), and 5 exception(s). View
6.03 KB

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

Status: Needs review » Needs work

The last submitted patch, 138: d8_entity_validate_general.patch, failed testing.

jibran’s picture

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

fago’s picture

Status: Needs work » Needs review
FileSize
44.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,396 pass(es). View
789 bytes

Not addressed #140 yet.

larowlan’s picture

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

larowlan’s picture

+++ 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

larowlan’s picture

FileSize
4 KB
44.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,397 pass(es). View

fixes 140 and some other minor nits

fago’s picture

dawehner’s picture

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

fago’s picture

Assigned: fago » Unassigned
FileSize
3.01 MB
3.05 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch d8_validator_5.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 147: d8_validator.patch, failed testing.

dawehner’s picture

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

fago’s picture

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.

webchick’s picture

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.

fago’s picture

@webchick: Thanks, makes sense.

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

fago’s picture

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.

effulgentsia’s picture

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?

fago’s picture

Status: Needs work » Needs review
FileSize
54.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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.

fago’s picture

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

Status: Needs review » Needs work

The last submitted patch, 155: d8_validator_recursive.patch, failed testing.

jibran’s picture

Let's update the IS after #155

dawehner’s picture

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.

dawehner’s picture

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

dashaforbes’s picture

Status: Needs work » Needs review
FileSize
23.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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

dawehner’s picture

FileSize
62.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
13.92 KB

@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.

The last submitted patch, 161: d8_validator_recursive-161.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 162: 2343035-162.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
739 bytes
62.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,366 pass(es), 207 fail(s), and 19 exception(s). View

Applies 161 to 162

Status: Needs review » Needs work

The last submitted patch, 165: validator-2.5-2343035-4.165.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
70.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
33.72 KB

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.

fago’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 167: d8_validator.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
70.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.42 KB

Status: Needs review » Needs work

The last submitted patch, 170: d8_validator.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
69 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
7.92 KB

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.

Status: Needs review » Needs work

The last submitted patch, 172: d8_validator.patch, failed testing.

dashaforbes’s picture

Status: Needs work » Needs review

Thanks @dawehner

The last submitted patch, 122: 2343035-122.patch, failed testing.

dawehner’s picture

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

dawehner’s picture

FileSize
69.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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).

webmozart’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 177: 2343035-177.patch, failed testing.

fago’s picture

@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.

dawehner’s picture

FileSize
9.31 KB

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

bojanz’s picture

   /**
    * {@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).

dawehner’s picture

Status: Needs work » Needs review
FileSize
69.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,877 pass(es). View
2.86 KB

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

dawehner’s picture

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 :)

dawehner’s picture

FileSize
67.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,924 pass(es). View
12.76 KB

Fixing a couple of those nitpicks.

fago’s picture

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.

dawehner’s picture

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.

fago’s picture

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

dawehner’s picture

FileSize
72.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,882 pass(es). View
5.97 KB

Ah yeah the problem was the test.

dawehner’s picture

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

jibran’s picture

+++ 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?

dawehner’s picture

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.

fago’s picture

Issue summary: View changes
FileSize
72.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,938 pass(es). View
1.66 KB
  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’s picture

@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.

dawehner’s picture

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.

fago’s picture

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

I see, fine to me!

dawehner’s picture

FileSize
76.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
15.2 KB
  • 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.
dawehner’s picture

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

dawehner’s picture

FileSize
78.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,936 pass(es). View
1.49 KB

Forgot the interface file.

The last submitted patch, 197: 2343035-197.patch, failed testing.

klausi’s picture

Status: Needs review » Needs work
  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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
78.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,955 pass(es). View
1.54 KB

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!

jibran’s picture

FileSize
3.29 KB
81.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,944 pass(es). View

Yay!!! this is green. Some doc update suggestions we can move this to followup if these are too much. Fixed some nits as well.

  1. +++ b/core/lib/Drupal/Core/TypedData/Validation/ContextualValidatorInterface.php
    @@ -0,0 +1,36 @@
    +   * {@link \Symfony\Component\Validator\Constraints\Valid} is assumed.
    

    This will not play well with api.d.o.

  2. +++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContext.php
    @@ -0,0 +1,317 @@
    + * We do not use the context provided by Symfony as it is marked internal.
    

    Can we please explain the differences from Symfony/ExecutionContext here as well? Can we add @see \Drupal\Core\TypedData\Validation\ExecutionContextFactory::createContext() as well?

  3. +++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContext.php
    @@ -0,0 +1,317 @@
    +   * @internal Called by {@link ExecutionContextFactory}. Should not be used
    +   *           in user code.
    

    I think we have our own ExecutionContextFactory now so this can be updated.

  4. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,222 @@
    +/**
    + * Defines a recursive contextual validator for Typed Data.
    + */
    +class RecursiveContextualValidator implements ContextualValidatorInterface {
    

    Can we add @see \Drupal\Core\TypedData\Validation\RecursiveValidator::startContext() and \Drupal\Core\TypedData\Validation\RecursiveValidator::inContext() here?

  5. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,222 @@
    +    // @todo This value is not used at the moment.
    

    Can we fix this @todo here or do we need an issue for this?

  6. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,222 @@
    +   * If no constraints are passed, the data is validated against the
    +   * constraints specified in its data definition. If the data is complex or a
    +   * list and no constraints are passed, the contained properties or list items
    +   * are validated recursively.
    

    Can we add this kind of docs to class desc as well?

  7. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php
    @@ -0,0 +1,119 @@
    +class RecursiveValidator implements ValidatorInterface {
    

    Can we please explain the difference here between this and Symfony\RecursiveValidator?

  8. +++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php
    @@ -0,0 +1,75 @@
    +/**
    + * Validator metadata for typed data objects.
    + */
    +class TypedDataMetadata implements MetadataInterface {
    

    Can we put @see \Drupal\Core\TypedData\Validation\RecursiveValidator::getMetadataFor() here?

  9. +++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php
    @@ -0,0 +1,75 @@
    + * Validator metadata for typed data objects.
    

    Can we please explain here what's the purpose of this class and why is it different from GenericMetadata etc.?

  10. I think we can keep the formatting of \Symfony\Component\Validator\Violation\ConstraintViolationBuilder in \Drupal\Core\TypedData\Validation\ConstraintViolationBuilder.
  11. I think we can keep the formatting of \Symfony\Component\Validator\Context\ExecutionContextFactory in \Drupal\Core\TypedData\Validation\ExecutionContextFactory.
jibran’s picture

dawehner’s picture

Work on the great feedback from jibran and take care about the upstream change.

klausi’s picture

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

    And we are still casting the empty string here.

  2. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,222 @@
    +    // @todo This value is not used at the moment.
    +    $this->defaultPropertyPath = $this->context->getPropertyPath($path);
    

    Yeah, this should be fixed. $this->defaultPropertyPath is not documented as property on the class. We should remove this line and add a comment like "Ignore any call to this method".

  3. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,222 @@
    +    // You can pass a single constraint or an array of constraints
    +    // Make sure to deal with an array in the rest of the code
    

    Missing fullstop on both lines.

  4. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php
    @@ -0,0 +1,119 @@
    +    $class = '\Drupal\Core\TypedData\Validation\TypedDataMetadata';
    +    return new $class($typed_data);
    

    Why do you need the $class variable here? You can just use the class name directly?

klausi’s picture

  1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/UniqueFieldValueValidator.php
    @@ -19,7 +19,7 @@ class UniqueFieldValueValidator extends ConstraintValidator {
    -    if (!isset($items)) {
    +    if (!$item = $items->first()) {
    

    Can we split this into 2 lines? I hate assignments in if() clauses.

  2. +++ b/core/tests/Drupal/Tests/Core/TypedData/RecursiveContextualValidatorTest.php
    @@ -0,0 +1,347 @@
    +/**
    + * Tests \Drupal\Core\TypedData\Validation\RecursiveContextualValidator
    + *
    + * @group typedData
    + * @coversDefaultClass \Drupal\Core\TypedData\Validation\RecursiveContextualValidator
    + */
    

    You can remove the first line, @coversDefaultClass already tells me that.

  3. +++ b/core/tests/Drupal/Tests/Core/TypedData/RecursiveContextualValidatorTest.php
    @@ -0,0 +1,347 @@
    +  /**
    +   * @covers ::validate
    +   *
    +   * @expectedException \Exception
    +   */
    

    Why \Exception and not the specific exception we throw there? Please add a comment.

Otherwise this looks pretty good!

dawehner’s picture

Status: Needs work » Needs review
FileSize
79.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,186 pass(es). View
14.72 KB

Can we fix this @todo here or do we need an issue for this?

Sure, here is one #2482527: Figure out whether we want to support \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::atPath

Can we please explain here what's the purpose of this class and why is it different from GenericMetadata etc.?

I think that is kinda pointless, ... its a metadata implementation specific for type data, the generic metadata is entirely different.

I think we can keep the formatting of \Symfony\Component\Validator\Violation\ConstraintViolationBuilder in \Drupal\Core\TypedData\Validation\ConstraintViolationBuilder.

Well sure, we can do that. Let's see how blocks the RTBC for that later :P

The upstream PR is merged https://github.com/symfony/symfony/commit/f6c77ff362d39df7c7418f940ac850....

Skipped that part for now, as https://packagist.org/packages/symfony/validator does not yet have the more recent commit hash, grrr, so much time, no win.

Can we split this into 2 lines? I hate assignments in if() clauses.

But don't we use that pattern in gazilions of places in core? I don't dislike this pattern.

fago’s picture

I think we can keep the formatting of \Symfony\Component\Validator\Violation\ConstraintViolationBuilder in \Drupal\Core\TypedData\Validation\ConstraintViolationBuilder.

Not sure. The classes are rather trivial, but as soon as we do that we need to note they are copied from Symfony what means we'll have to take over the copyright mention also. So imo either that, or provide our own version in Drupal style.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

@dawehner Thank you for appreciating my review. :)
I'm fine #210. Either Drupal cs or Symfony cs. I don't think mixture of both is a good thing. I think this is ready we can make a non-critical follow up for upstream fix.

+++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
@@ -65,9 +71,8 @@ public function __construct(ExecutionContextInterface $context, MetadataFactoryI
+    // @todo This mmethod is not used at the moment, see

mmethod. :)

dawehner’s picture

Not sure. The classes are rather trivial, but as soon as we do that we need to note they are copied from Symfony what means we'll have to take over the copyright mention also. So imo either that, or provide our own version in Drupal style.

Yeah, well, for now this is a version of the patch with some ignoring of the codestyle, see https://pear.php.net/manual/en/package.php.php-codesniffer.advanced-usag...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContext.php
    @@ -0,0 +1,319 @@
    + * as execeptions for methods we don't support.
    

    Spelling mistake - exceptions

  2. +++ b/core/lib/Drupal/Core/TypedData/Validation/ExecutionContextFactory.php
    @@ -0,0 +1,60 @@
    + * We do not use the factory provided by Symfony as it is marked internal.
    ...
    +    return new ExecutionContext(
    

    So it ExecutionContext...

  3. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +    $this->context->setNode($value, $data, $metadata, $property_path);
    ...
    +    if (isset($constraints) || !$this->context->isGroupValidated($cache_key, Constraint::DEFAULT_GROUP)) {
    ...
    +    $this->context->setNode($previous_value, $previous_object, $previous_metadata, $previous_path);
    

    setNode is marked as internal

  4. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +    if (isset($constraints) || !$this->context->isGroupValidated($cache_key, Constraint::DEFAULT_GROUP)) {
    

    isGroupValidated is marked as internal

  5. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +        $this->context->markGroupAsValidated($cache_key, Constraint::DEFAULT_GROUP);
    

    markGroupAsValidated is marked as internal

  6. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +  /**
    +   * Validates a node's value against all constraints in the given group.
    +   *
    +   * @param mixed $value
    +   *   The validated value.
    +   */
    +  protected function validateConstraints($value, $cache_key, $constraints) {
    

    Some params need docs

  7. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +        if ($this->context->isConstraintValidated($cache_key, $constraint_hash)) {
    

    isConstraintValidated is marked as internal

  8. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +        $this->context->markConstraintAsValidated($cache_key, $constraint_hash);
    

    markConstraintAsValidated is internal

  9. +++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
    @@ -0,0 +1,227 @@
    +      $this->context->setConstraint($constraint);
    

    setConstraint is internal

  10. +++ b/core/lib/Drupal/Core/TypedData/Validation/TypedDataAwareValidatorTrait.php
    @@ -0,0 +1,37 @@
    +use Drupal\Core\TypedData\PrimitiveInterface;
    

    Not used.

  11. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php
    @@ -9,6 +9,8 @@
    +use Drupal\Core\TypedData\Validation\TypedDataPropertyValidationEnvelope;
    

    Does not exist and therefore not used.

With using methods and classes marked as internal I think we might just need to add a comment to explain why.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.79 KB
33.71 KB
106.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,212 pass(es). View

Thank you for the review.

Note: This patch now also updates symfony validator and ensures that we use the new property path from symfony itself again.

So it ExecutionContext...

Can you clarify what you means with that?

setNode is marked as internal

isGroupValidated is marked as internal

markGroupAsValidated is marked as internal

This is fine, let me quote the @internal tag on the interface \Symfony\Component\Validator\Context\ExecutionContextInterface::setNode:

* @internal Used by the validator engine. Should not be called by user
* code.

We are the validator engine ... and we implement exactly because of this reason the execution context interface ...

Some params need docs

Fixed that

larowlan’s picture

FileSize
1009 bytes
83.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,986 pass(es), 33 fail(s), and 26 exception(s). View
83.11 KB

Minor tweak to the wording but rtbc+1.
I now have 5 local branches for this issue, new record and time for a cleanup

larowlan’s picture

FileSize
106.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,218 pass(es). View

wrong file

jibran’s picture

I think spelling mistake mentioned in #211 is still pending.

The last submitted patch, 215: upgrade_validator-2343035-215.patch, failed testing.

dawehner’s picture

@jibran
I think its not, see

+   */
+  public function atPath($path) {
+    // @todo This method is not used at the moment, see
+    //   https://www.drupal.org/node/2482527
+    return $this;
+  }
+
jibran’s picture

Status: Needs review » Reviewed & tested by the community

Sorry @dawehner I think I missed that in interdiff. I think feedback from @alexpott is addressed. Let's get this fixed.

ianthomas_uk’s picture

So it ExecutionContext...

Can you clarify what you means with that?

I guess he meant "So *is* ExecutionContext". It does seem a little odd that sometimes it is OK for us to use things marked as internal and sometimes it isn't, but maybe the difference would be obvious if I read the patch more closely.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I wondered if we should add @internal to our classes - but this would start a waterfall of annotations so leaving alone for now.

Committed 373d925 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php b/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php
old mode 100755
new mode 100644
diff --git a/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php
old mode 100755
new mode 100644

Fixed some file perms on commit.

  • alexpott committed 373d925 on 8.0.x
    Issue #2343035 by fago, dawehner, larowlan, ianthomas_uk, cilefen,...
jibran’s picture

Published the change notice. Thank you @alexpott for committing this. Un-postponed the #2105797: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered.
Thank you everyone especially @dawehner, @fago, @larowlan and @webmozart.

webmozart’s picture

Glad to see this fixed :)

fago’s picture

Great to see this one land, thank you everyone!

Status: Fixed » Closed (fixed)

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