#2144327: Make all field types provide a schema() added a call to getFieldType which doesn't even exist. Then it tries to add an array to a NULL which fatals. There are, obviously, no tests. I haven't written any but now that migrate uses this, that's ample test... Migrate just does this:

      $schema = FieldDefinition::create($type)->getColumns();
      return $schema[$column];

Also, StringItem tries to get max_length by running $field_definition->getSetting('max_length') but at this moment $field_definition has no settings. If I patch up FieldDefinition::schema to do

      $this->setSettings($definition['settings']);
      $schema = $class::schema($this);

then it works but it's clearly wrong -- if someone tries to run just FieldDefinition::create('string')->getSetting('max_length') that wouldn't work. Not sure what to do here. TypeData is still very strange to me.

CommentFileSizeAuthor
#7 2174509-test-only.patch1.15 KBamateescu
#7 2174509-7.patch2.18 KBamateescu
urgh.patch1.04 KBchx

Comments

tim.plunkett’s picture

It used to exist, it was renamed in #2143263: Remove "Field" prefix from FieldDefinitionInterface methods.
So, missing test coverage?

chx’s picture

Absolutely. The new schema stuff went in weeks after the linked issue. I also noted the lack of test coverage in the issue summary.

chx’s picture

Issue summary: View changes
yched’s picture

FieldDefinition::create('string')->getSetting('max_length') is real, but different issue.

Opened #2175017: FieldDefinition::create() doesn't populate default 'settings' for the field type.

chx’s picture

Well, it might be but that's essentially what the current code runs.

yched’s picture

Yeah, sorry, this is definitely related - but larger issue, better discussed in a separate task IMO.

amateescu’s picture

Title: The new schema stuff is broken » Drupal\Core\Field\FieldDefinition::getSchema() is broken
Issue tags: +Entity Field API
StatusFileSize
new2.18 KB
new1.15 KB

Here's a quick test. I thought the original patch tested this enough through the existing field tests but, obviously, I haven't considered base fields :(

Status: Needs review » Needs work

The last submitted patch, 7: 2174509-test-only.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, let's get this in please. Patch looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fde84e2 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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