Problem/Motivation

#2869809: Make it easy to get typed config representations of entities introduced \Drupal\Core\Config\TypedConfigManager::createFromNameAndData():

  public function createFromNameAndData($config_name, array $config_data) {
    $definition = $this->getDefinition($config_name);
    $data_definition = $this->buildDataDefinition($definition, $config_data);
    return $this->create($data_definition, $config_data);
  }

It's great! It's used in many places.

But it has one bug: the \Drupal\Core\TypedData\TraversableTypedDataInterface instances it returns do not have a working getName() nor a working getPropertyPath(), because we forgot to pass the third argument to ::create(): public function create(DataDefinitionInterface $definition, $value = NULL, $name = NULL, $parent = NULL) { — the $name!

Steps to reproduce

N/A — discovered in #3216089: Expose validation constraints (and validatability %) in Config Inspector UI.

Proposed resolution

Specify $name.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Active » Needs review
StatusFileSize
new1.15 KB
new1.9 KB
wim leers’s picture

@borisson_ raised a legitimate concern in Drupal Slack:

hm, it's kind of worrying that this has been in core for years and has only surfaced now, would it be an idea to be more defensive in the create method so that we alert people who are creating these objects when they are incomplete?

It’s been in core for years but that method is called virtually nowhere. It pretty much only exists in Typed Config tests!

Since CKEditor 5 is in core, 3 calls to it were added to core. Which brings it to a grand total of 12 🤣

The thing is that the commit that introduced it (#2869809: Make it easy to get typed config representations of entities) was specifically done as one of the blockers for POST and PATCH support for config entities via the REST module.

But of course the validation pieces never materialized … so none of that really happened.

That’s why I’m running into it now: because I’m working on validation 🤓

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Discussed with Wim in slack, I now understand why this only breaks in this scenario, patch looks great!

The last submitted patch, 2: 3360991-2-test-only-FAIL.patch, failed testing. View results

wim leers’s picture

wim leers’s picture

Issue summary: View changes

  • lauriii committed 96e2fd9d on 11.x
    Issue #3360991 by Wim Leers, borisson_: TypedData instances created by...

  • lauriii committed 9050ec54 on 10.1.x
    Issue #3360991 by Wim Leers, borisson_: TypedData instances created by...
lauriii’s picture

Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 96e2fd9 and pushed to 11.x. Also cherry-picked to 10.1.x. Thanks!

Interestingly, yesterday I was debugging code in #3358049: Save FieldStorageConfig at the same time as FieldConfig where I had done the same mistake 😅 It did take me a while to realize until I found yet another map in \Drupal\Core\TypedData\TypedDataManager::$prototypes.

Status: Fixed » Closed (fixed)

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