Problem/Motivation
If a map data definition has required properties but is given an empty array as
its value, the validator skips it entirely and reports no violations. The
required properties are never checked because
RecursiveContextualValidator::validateNode() guards recursion with
!$data->isEmpty(), and an empty map is considered empty regardless
of whether a value was explicitly provided.
The root cause is that Map::$values defaulted to [],
making a map that was never given a value indistinguishable from one
explicitly set to []. Both returned TRUE from
isEmpty(), so both were silently skipped.
Steps to reproduce
use Drupal\Core\TypedData\DataDefinition;
use Drupal\Core\TypedData\MapDataDefinition;
$typed_data_manager = \Drupal::typedDataManager();
$definition = MapDataDefinition::create()
->setPropertyDefinition('foo', DataDefinition::create('string')
->setRequired(TRUE))
->setPropertyDefinition('bar', DataDefinition::create('string')
->setRequired(TRUE));
$invalid_but_passes = $typed_data_manager->create($definition, []);
// Should report two violations (foo and bar are missing).
// Actually reports 0.
count($invalid_but_passes->validate()); // 0
Proposed resolution
Two coordinated changes:
1. Change Map::$values to default to NULL.
This lets the validator tell apart a map that was never given a value
(NULL, should be skipped) from one explicitly given an empty array
([], must validate its required properties). FieldItemBase
overrides the default back to [] so the field layer is
completely unaffected.
2. Update RecursiveContextualValidator::isEmptyMapWithValue().
Instead of a separate hasValue flag, the helper now checks
$data->getDataDefinition() instanceof MapDataDefinition &&
$data->getValue() !== NULL. Using the definition type rather than
the class avoids importing the field API into the validator: field items
use FieldItemDataDefinition, which does not extend
MapDataDefinition, so they fall out of the check naturally.
EntityUntranslatableFieldsConstraintValidator also receives a
NULL guard (matching the pattern of all other entity constraint
validators), since recursing into a map whose entity property is missing now
instantiates that property with NULL.
Remaining tasks
None.
User interface changes
None.
API changes
Map::getValue() now returns NULL instead of
[] when no value has been set on the map. Code that compared the
return value to [] should be updated to check
!== NULL instead. Field items are not affected. See the change
record for details.
Data model changes
None.
Release notes snippet
Typed data maps now validate their required properties when given an empty
array. Before, an empty map passed validation even when required properties
were missing. Map also defaults to NULL instead of an empty array, so a map
created without a value now returns NULL from getValue(). Field items are not
affected, but maps nested inside field items are, such as the options property
on link fields. See the change record for what to update.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3256536
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
mglamanComment #3
mglamanComment #10
attilatilman commentedComment #11
attilatilman commentedComment #12
attilatilman commentedComment #13
mglamanComment #14
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #15
mglamanI have one nit on the inline comment, but setting to RTBC
Comment #16
mglamanComment #17
fagoThanks!
I think it's wrong to special case "MapDataDefinition" here - the problem seems to be generic to everyy complex-data, not only maps.
Skipping validation for empty lists seems wright, but skipping validation for empty complex-data is what seems wrong. I think it would better to different between "unset" and empty ([]" for maps/complex-data and generally skip validation only if complex data is unset (value===NULL). So an unset map-property which is not required is ignored, an unset map-property which is required would be marked missing. An un-required empty-map property with required props needs to fail validation, sense "empty" is not valid if props are required.
Thus, I'd propose to:
- Only skip validating complex data if value is === NULL, keep validating if they are empty.
- Keep the default of [] - it's not required to be changed.
I've also checked with how JSON-schema handles this - and it'S the same. Unset items are skipped, but an empty [] is validated. So this seems to be inline with the rest of the world does then.