Document in great detail where constraints for data leafs is authoritatively defined

Created on 2 July 2024, 4 months ago
Updated 19 July 2024, 3 months ago

Problem/Motivation

Found in πŸ“Œ [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed
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

πŸ› Bug report
Status

Fixed

Component

Data model

Created by

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @tedbow
  • Merge request !74Issue #3458580 fix missing constraints β†’ (Merged) created by tedbow
  • Issue was unassigned.
  • Status changed to Needs review 4 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    Marking this as Critical as blocks πŸ“Œ [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed which is critical

  • Assigned to tedbow
  • Status changed to Needs work 3 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
    1. What does "all … are not found" mean? πŸ€” Do you mean "not all … are found"?
    2. 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 🀯

    3. 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? πŸ€·β€β™‚οΈ
  • Issue was unassigned.
  • Status changed to RTBC 3 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    If after this docs + logic improvement, πŸ“Œ [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed 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.

  • Pipeline finished with Skipped
    3 months ago
    #216734
  • Status changed to Fixed 3 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024