Make 'sequence' Typed Data instances return type of 'sequence' instead of 'list'

Oops! 😅

Discovered this in ✨ Expose validation constraints (and validatability %) in Config Inspector UI Fixed and hence had to work around this.

How the hell did you run into this?

I worked around it in contrib like this:

  /**
   * Work-around for bug introduced in #2361539.
   *
   * @see \Drupal\Core\TypedData\ListDataDefinition::getDataType()
   * @see \Drupal\Core\Config\Schema\SequenceDataDefinition
   * @see https://www.drupal.org/project/drupal/issues/2361539
   * @todo Remove this when this module requires a Drupal core version that includes https://www.drupal.org/project/drupal/issues/3361034.
   */
  protected static function getDataType(TypedDataInterface $typed_data) : string {
    $data_type = $typed_data->getDataDefinition()->getDataType();
    if ($data_type === 'list') {
      $data_type = 'sequence';
    }
    return $data_type;
  }

… because without this, it's impossible to correctly traverse a typed data tree by using the return value of $typed_data->getDataDefinition()->getDataType() … since list is not actually a config schema type, \Drupal\Core\Config\TypedConfigManager::determineType() decides to make it automagically fall back to the undefined type 😳

… at which point traversing further down the three of course is impossible.

Why can this go undiscovered for a decade?

The reason this has not caused problems for Drupal core so far is that \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validateNode() does not use the full config schema tree, but instead relies on inspecting the typed data's object instances:

    // If the data is a list or complex data, validate the contained list items
    // or properties. However, do not recurse if the data is empty.
    // Next, we do not recurse if given constraints are validated against an
    // entity, since we should determine whether the entity matches the
    // constraints and not whether the entity validates.
    if (($data instanceof ListInterface || $data instanceof ComplexDataInterface) && !$data->isEmpty() && !($data instanceof EntityAdapter && $constraints_given)) {
      foreach ($data as $property) {
        $this->validateNode($property);
      }
    }

Super interesting bug, good find. Are the other TypedData classes correct?

Are the other TypedData classes correct?

FieldConfigBase::getDataType() is wrong too:

  public function getDataType() {
    return 'list';
  }

… but I figured we'd keep this tightly scoped, because that is one of the most complex pieces of config 😱

The last submitted patch, 3: 3361034-3-test-only-FAIL.patch, failed testing. View results →

The last submitted patch, 3: 3361034-3.patch, failed testing. View results →

Looks like a random failure to me

Triaging the RTBC queue.

What a easy issue to triage! The IS is clear and enjoyable to read. The situation is further explained in comment #5. I also agree with the scoping. All questions have been answered.

The only thin I see that it does appear this a followup is needed for #7.

I have updated the title to explain what is being changed.

#11 That's LOVELY to hear! 😊😊 Glad to read it helps and is appreciated!

New issue created, with a lot more detail than was present in #7: 🐛 Clarify why FieldConfigBase::getDataType() is 'list' and not 'field_config_base' Fixed .

Good find! Yes, the typed-data implementation of config wasn't focused enough on implementing its contract cleanly.

The fix is straight forward, the right thing todo and comes with test! I second the RTBC!

Discussed with @longwave and @fago and agreed that we should write a short change record for the behavior change here.

I've added a change record draft

lauriii → credited longwave → .

Committed 89ac074 and pushed to 11.x. Thanks!

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