Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mariancalinro’s picture

mariancalinro’s picture

Status: Active » Needs review
FileSize
2.8 KB
fago’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Validation/ComplexDataConstraintValidatorTest.php
    @@ -0,0 +1,74 @@
    +      'name' => 'Tests ComplexData validation constraint',
    

    It's all tests, so "Complex data constraint" should be enough.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Validation/ComplexDataConstraintValidatorTest.php
    @@ -0,0 +1,74 @@
    +
    

    unnecessary empty line

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Validation/ComplexDataConstraintValidatorTest.php
    @@ -0,0 +1,74 @@
    +   * Tests the ComplexData validation constraint validator by creating a
    +   * typedData map definition with a ComplexData constraint containing one
    +   * AllowedValues constraint, and then by tring to create a typedData object
    +   * with both an allowed and a dissalowed value.
    

    Summary should fit in 80chars, so best split it in a summary and additional explanation.

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Validation/ComplexDataConstraintValidatorTest.php
    @@ -0,0 +1,74 @@
    +            'AllowedValues' => array(1,2,3)
    

    Misses spaces after the commas.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Validation/ComplexDataConstraintValidatorTest.php
    @@ -0,0 +1,74 @@
    +    $typed_data = $this->typedData->create($definition, array('key' => 1));
    

    I'm wondering what happens when we pass a map without the "key" being set.

    Imo, it should be ignored when the value is not set as you have to add a NotNull constraint for what should be required. Howsoever, we should add a case for that as well.

fago’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
3.38 KB

Adressed the remarks and extended the test-coverage for the case mentioned in 5.

fago’s picture

yched’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/lib/Drupal/system/Tests/Validation/ComplexDataConstraintValidatorTest.php
@@ -0,0 +1,86 @@
+    $this->assertEqual($violations->count(), 0, 'Not existing key is ignored.');

Minor: s/Not existing key /Constraint on non-existing key/ ?

Otherwise, same as #2142981-11: Test for AllowedValuesConstraintValidator : would be really nice to get #2132145: Rename 'typed_data' / Drupal::typedData() to 'typed_data_manager' / Drupal::typedDataManager committed...

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: d8_complex_data_constraint.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
1.19 KB

Addressed #6 and updated to account for the rename.

yched’s picture

Status: Needs review » Reviewed & tested by the community

$this->typedData could be renamed accordingly ;-) The point of the rename wsa to remove the naming confusion between data items and their factoty.

Nitpick though. Otherwise RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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