Skip to content
Snippets Groups Projects

Added constraint validator to taxonomy.settings:terms_per_page_admin

Added constraint validator to taxonomy.settings:terms_per_page_admin
issue/drupal-3395563:3395563-add-validation-constraints into 11.x
4 open threads
Closed
Borja Vicenterequested to merge
issue/drupal-3395563:3395563-add-validation-constraints into 11.x
4 open threads

Closes #3395563

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/TypeConstraint.php 0 → 100644
3 namespace Drupal\Core\Validation\Plugin\Validation\Constraint;
4
5 use Symfony\Component\Validator\Constraints\Type;
6 use Symfony\Component\Validator\Constraints\TypeValidator;
7
8 /**
9 * Type constraint.
10 *
11 * Extends Symfony's Type constraint to validate types.
12 *
13 * @Constraint(
14 * id = "Type",
15 * label = @Translation("Type constraint", context = "Validation"),
16 * )
17 */
18 class TypeConstraint extends Type {
  • Lauri Timmanee
    Lauri Timmanee @lauriii
    Maintainer

    Isn't this something we already have in \Drupal\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraint?

  • Borja Vicente
    Borja Vicente @nireneko
    Author

    I think that constraint is for TypedData primitive types, because in the trait that the validator uses there is one "if" checking if the value is instance of \Drupal\Core\TypedData\TypedDataInterface.

  • Marco Villegas
    Marco Villegas @marvil07

    Yes, Drupal\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidator is intended only for typed data, as the checks on its code suggest. In other words a raw language type will not be validated with them, instead only the Drupal\Core\TypedData\Type\*Interface types.

  • Wim Leers
    Wim Leers @wimleers
    Developer

    :thumbsdown: @lauriii is right: we don't need this constraint. \Drupal\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidator::validate() already handles this.

    Why? Because when config is validated against its schema, it is always converted to TypedData.

    See PrimitiveTypeConstraintValidatorTest and ConfigSchemaTest::testConfigSaveWithSchema(). Try changing the 'integer' => '100', test value to something else, and you'll see it gets cast to an integer anyway. Changing that may be something we some day want to do, but is very out of scope here.

  • Borja Vicente changed this line in version 6 of the diff

    changed this line in version 6 of the diff

  • Borja Vicente
    Borja Vicente @nireneko
    Author

    @wimleers @lauriii removed TypeConstraint and now is using "PrimitiveType".

    But if the configuration is validated converting it to TypedData, probably we don't need that constraint in *.schema.yml.

    And thanks for the explanation.

  • Please register or sign in to reply
  • Lauri Timmanee
    Lauri Timmanee @lauriii started a thread on the diff
    • core/modules/taxonomy/config/schema/taxonomy.schema.yml
    13 13 terms_per_page_admin:
    14 14 type: integer
    15 15 label: 'Number of terms per page'
    16 constraints:
  • Marco Villegas
    Marco Villegas @marvil07 started a thread on an old version of the diff
    • core/modules/taxonomy/config/schema/taxonomy.schema.yml
    13 13 terms_per_page_admin:
    14 14 type: integer
    15 15 label: 'Number of terms per page'
    16 constraints:
    17 Type:
    18 type: 'integer'
    • Comment on lines +17 to +18
      Marco Villegas
      Marco Villegas @marvil07
      Suggested change
      Applied
      17 Type:
      18 type: 'integer'
      17 Type:
      18 type: 'integer'
      19 Range:
      20 min: 0

      The Range constraint validator plugin provides this functionality.

      BTW the related Drupal\Core\Validation\Plugin\Validation\Constraint\RangeConstraintValidator has at least a is_numeric() check.

    • Borja Vicente
      Borja Vicente @nireneko
      Author

      Great, thanks :smile:

      But should be min: 1 to force higher than 0?

      About is_numeric(), acording to php documentation (https://www.php.net/manual/en/function.is-numeric.php), that function will return true for float.

    • Marco Villegas
      Marco Villegas @marvil07

      But should be min: 1 to force higher than 0?

      Yes, added on the last commit. Looking at where this is used, I do not see any special meaning associated with 0, as in other places, so yes :thumbsup:

      About is_numeric and float, yes, that is the case. The way I think about constraints is as composable blocks, complementing each other. In that way, the Type plugin checks the language type, and the Range plugin check the minimal value here.

    • Borja Vicente changed this line in version 6 of the diff

      changed this line in version 6 of the diff

    • Please register or sign in to reply
  • Marco Villegas added 1 commit

    added 1 commit

    • fb0107a8 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Marco Villegas added 1 commit

    added 1 commit

    • 65abcb73 - Set minimal term list size to 1

    Compare with previous version

  • Borja Vicente added 1 commit

    added 1 commit

    • a8719be9 - Removed unnecessary constrain.

    Compare with previous version

  • Wim Leers
    Wim Leers @wimleers started a thread on an old version of the diff
    • core/modules/taxonomy/config/schema/taxonomy.schema.yml
    13 13 terms_per_page_admin:
    14 14 type: integer
    15 15 label: 'Number of terms per page'
    16 constraints:
    17 PrimitiveType: []
    • Wim Leers
      Wim Leers @wimleers
      Developer
      Suggested change
      17 PrimitiveType: []

      This is not necessary. type: integer automatically gets this constraint applied, like I explained earlier. Search the codebase for 'PrimitiveType' (exactly this string, including the single quotes), and you'll find the place where it gets automagically added.

      If we'd need this here, we'd need this on literally every config schema definition (!!!) for a scalar (string/int/float/bool or specialization of any of these) value!

    • Borja Vicente changed this line in version 7 of the diff

      changed this line in version 7 of the diff

    • Please register or sign in to reply
  • Borja Vicente added 1 commit

    added 1 commit

    • 4dc8dfd0 - Removed constraint of PrimitiveType.

    Compare with previous version

  • Lauri Timmanee added 128 commits

    added 128 commits

    • 4dc8dfd0...82c59f84 - 121 commits from branch project:11.x
    • 20ca4a8d - Added constraint validator to taxonomy.settings:terms_per_page_admin
    • fc21ca7a - Added new constraint to check types.
    • 21518b23 - Fixed unused use statement
    • f33256e3 - Apply 1 suggestion(s) to 1 file(s)
    • 12c38130 - Set minimal term list size to 1
    • 2b092ac2 - Removed unnecessary constrain.
    • 949ed985 - Removed constraint of PrimitiveType.

    Compare with previous version

    Toggle commit list
  • Lauri Timmanee closed

    closed

  • Marco Villegas mentioned in merge request !5675

    mentioned in merge request !5675

    • Please register or sign in to reply