Problem/Motivation

Found in #3456008: [later phase] Support matching enum SDC prop shapes against DynamicPropSources, not only generating StaticPropSources
All field item constraints are not found when evaluating prop matches. There are couple problem with this.

  1. $complex_data_constraint = array_filter(
          $field_item->getConstraints(),
          fn ($c) => $c instanceof ComplexDataConstraint
        );
        if (!empty($complex_data_constraint)) {
          $field_item_level_constraints_indirect = reset($complex_data_constraint)
            ->properties[$field_property_name] ?? [];
        }
        else {
          $field_item_level_constraints_indirect = [];
        }
    

    $complex_data_constraint is an array but we only check the first element in the array. I have worked on temporary fix in #3456008 that I will bring in here and it does find more constraints

  2. $rekey = function (array $constraints) {
          return array_combine(
            array_map(
              fn (Constraint $c): string => get_class($c),
              $constraints,
            ),
            $constraints
          );
        };
    

    This is assumes that classes among multiple constraints will be unique but this is not necessarily the case. I haven't checked if we have any cases of this yet though.

  3. $field_item_level_constraints_direct = $field_item->getConstraints()[$field_property_name] ?? [];
    

    I think the array returned from getConstraints() is actually always a list so this will never have a key of the property name. In #3456008 I did through an exception if this was to ever happen and it was never hit in our tests.

Steps to reproduce

Proposed resolution

Fix the bugs
Update the test expectations and see if they are reasonable

Remaining tasks

User interface changes

API changes

Data model changes

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

tedbow created an issue. See original summary.

tedbow’s picture

Assigned: tedbow » Unassigned
Priority: Normal » Critical
Status: Active » Needs review
wim leers’s picture

Assigned: Unassigned » tedbow
Status: Needs review » Needs work
Parent issue: » #3450586: [META] Back-end Kanban issue tracker
  • A. What does "all … are not found" mean? 🤔 Do you mean "not all … are found"?
  • B. This is indeed a tricky area, which is why there's this:
        // @todo Field item-level indirect vs direct constraints should not override each other. Investigate in Drupal core, this seems to be an oversight?
        // Field item-level constraints override property-level constraints.
    

    … because AFAICT Drupal core somehow does not (yet) provide infrastructure for this 🤯

RE: points in issue summary:

  1. Correct, "always using the first element" is what I discovered (during a deep debugging session) Drupal core always does … but now I wish I had added a comment pointing to where in Drupal core that is happening 😬
  2. Excellent remark — I didn't discover that edge case at all, but you're right that it's absolutely possible. We should at minimum add an assert() that verifies there's an empty intersection, and when we find the first non-empty case, we should match Drupal core's existing behavior. Ideally, Drupal core should provide an API for this …
  3. I'm fine with that simplification — I see that FieldItemBase::getConstraints() also promises this, so I have no idea why I did that. Probably defensive programming? 🤷‍♂️
wim leers’s picture

Title: All field item constraints are not found when evaluating prop matchs » Document in great detail where constraints for data leafs is authoritatively defined
Assigned: tedbow » Unassigned
Status: Needs work » Reviewed & tested by the community

If after this docs + logic improvement, #3456008: [later phase] Support matching enum SDC prop shapes against DynamicPropSources, not only generating StaticPropSources is still blocked, please open a new issue with a failing test case to show what the precise blocker is. This represents a step forward, so I'm going ahead and am merging this.

  • Wim Leers committed 373966b1 on 0.x authored by tedbow
    Issue #3458580 by tedbow, Wim Leers: Document in great detail where...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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