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.

Issue fork drupal-3256536

Command icon 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

mglaman created an issue. See original summary.

mglaman’s picture

Title: Map data definition properties are not validated if its empty » Map data definition properties are not validated if root value is empty
mglaman’s picture

Issue summary: View changes

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

attilatilman made their first commit to this issue’s fork.

attilatilman’s picture

Assigned: Unassigned » attilatilman
attilatilman’s picture

Issue summary: View changes
attilatilman’s picture

Issue summary: View changes
mglaman’s picture

Status: Active » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new1.05 KB

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

mglaman’s picture

Assigned: attilatilman » Unassigned
Status: Needs work » Reviewed & tested by the community

I have one nit on the inline comment, but setting to RTBC

mglaman’s picture

Issue tags: +rosetta
fago’s picture

Status: Reviewed & tested by the community » Needs work

Thanks!

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.