Make all field types provide a schema()

Here's the patch that I started while working on #2142549: Support multi-column field types in base tables. Didn't continue in there because there's a weird coupling of Drupal\field\FieldInterface with Drupal\Core\Field\FieldDefinitionInterface which doesn't make sense, so I thought we need a separate issue to sort this out.

Also, I think this is part of #2144263: Decouple entity field storage from configurable fields so I'll assign that as a parent.

this would be helpful for #1740492: Implement a default entity views data handler.

the patch didn't apply anymore FieldDefinition.php, attached is a re-roll without any other chances

The patch was createed on top of #2047229: Make use of classes for entity field and data definitions, which is a prerequisite anyway, that's why it didn't apply :)

ops, sorry for the noise then :)

One important thing we need to figure out here is the relationship of property definitions and the schema. Right now, we kind of assume schema column == property name everywhere if a property is stored. I think that's reasonable to assume, but still this should be documented to be required then.

So.. #2047229: Make use of classes for entity field and data definitions got in, let's get back to this patch. I don't see any alternative than duplicating the code for getSchema() and getColumns() between Drupal\field\Entity\Field and Drupal\Core\Field\FieldDefinition so maybe that needs to be cleaned-up in another issue, probably #2114707: Allow per-bundle overrides of field definitions?

Any proposal on where to document the relationship between property names and schema columns?

There we go. Does it make sense to postpone this on #2112239: Convert base field and property definitions?

Why do you feel it could be affected by #2112239: Convert base field and property definitions ?

Because I thought we might want to remove hardcoded things like 'not null', 'default', 'length' from these schemas and populate them based on the field definition and it would be nice to be able to use the class-based one :/

Fixed all those points.

Then I guess I'm just tired :P

Ok, I remembered what I was thinking initially and it's not ideal. I think we want to leave the population of those schema properties ('not null', 'default', 'length') to the code that creates the tables.

Because it needs to decide at the time when a module that provides an entity type is installed. After that, if some property of a base field of that entity type changes, the schema generated for it won't match what's already in the database.

I'm considering all this from the perspective of "auto-generate entity base tables" using base field schemas, and it's also very possible that I'm just overthinking it :)

Rerolled.

Because it needs to decide at the time when a module that provides an entity type is installed. After that, if some property of a base field of that entity type changes, the schema generated for it won't match what's already in the database.

Yeah, the schema is generated based on field definitions at install time. If you change your definitions, you have to take care of applying the schema changes. Sure it would be nice if we could generate updates.. - but that's out of scope for now. Consequently, I think we should include NOT NULL as needed.

But.. I'm not sure about default value. So far configurable fields did not have it, but base fields had. Still, default values are populated at API level anyway and are sometimes dynamic (time, language..). Any default values at db-level won't be picked up or used ever if you go with the API as you should - so what's the point in having and maintaining them?

1. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
   @@ -324,4 +332,55 @@ public function offsetSet($offset, $value) {
   +    // A typical use case for the method is to iterate on the columns, while
   +    // some other use cases rely on identifying the first column with the key()
   +    // function. Since the schema is persisted in the Field object, we take care
   +    // of resetting the array pointer so that the former does not interfere with
   +    // the latter.
   +    reset($schema['columns']);
   

   $this->schema is an array, i.e. it getSchema() returns a copy each time it's used. So is this really necessary?

2. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -24,6 +24,38 @@
   +   * possible to instantiate a FieldItemInterface object yet.
   

   This could use a little more clarification, as - as of now - you have all objects you need when you look at the interfaces. Also, for base fields you actually could do so.

   So maybe we should add that we cannot instantiate field items in every case as field instance settings might be missing.

3. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
   @@ -47,4 +48,11 @@ public function getPropertyDefinitions() {
   +   */
   +  public static function schema(FieldDefinitionInterface $field_definition) {
   +    return array();
   ...
   +
   

   We might want to document that computed field should return an empty array here, or even false?

Per http://buytaert.net/the-next-step-for-drupal-8-is-a-beta, if this is going to happen in D8, it should happen by beta.

$this->schema is an array, i.e. it getSchema() returns a copy each time it's used. So is this really necessary?

I looked at it with xdebug just to be sure, and it seems that it is indeed necessary. getSchema() does not return a copy because memory addresses were the same on subsequent calls to getColumns().

Fixed the other two points.

1. Yes, date is a date string just like the configurable one, so should have the same schema definition for that column. We additionally have the timestamp data type, which is an integer. The advantage of that is that it also implements DateTimeInterface, so you can get a DateTime object from it. There's currently no TimestampItem. Something that we want to add there too is a special field type for changed fields, that knows that it has to update itself in preSave().

7. I had that that problem too in #2023563: Convert entity field types to the field_type plugin, we then reverted the merge of the e-mail classes there for that reason. Yes, the constraint in the configurable e-mail item class is unnecessary and incomplete (it's the only e-mail validation that is applied there), without it, we do have the UX problem of not knowing to which field the error relates to. But there's a separate issue to fix that.

1. Fixed.
2. This is basically a revert of the interdiff from #17. I did it but I'm not sure it's the right thing to do because 'int' items also had 'default' => 0 to go along with 'not_null' => TRUE ...
3. Because parent:: is ConfigEntityReferenceItemBase and it's schema method is still the one copied from LegacyConfigFieldItem until #2015687: Convert field type to FieldType plugin for taxonomy module gets in.
4. Unfortunately, yes :(
5. Fixed.
6. Fixed.
7. Not sure what's the agreement here.. is any change needed or we're ok with two violations?

The problem is form API, which displayes validation messages at the top, *we* can map it correctly AFAIK. That's why we're messing with the validation error message and inject the field label. #2023465: Allow validation constraints to add in message replacements is the issue about that

I'd be fine (more than that, actually) with dropping that custom validation constraint, if we're ok with loosing that info until solve the issue above. As mentiones above, it only covers a tiny subset of validating an e-mail anyway.

I'd prefer keeping the second validation, at least as a reminder that there's something we need to fix.

Fixed the failures and #35.3.

And the correct patch this time.

+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -24,6 +24,40 @@
+   * Computed fields should return an empty array.

This needs to go on the field definition also. Then, I'd suggest rephrasing to:

"Computed fields having no schema should return an empty array."

plus @return should tell us that as well, e.g. An empty array if there is no schema, or ...

Like this?

So.. we want to rename it to getSchema() here since we're already touching most of the code?

I don't have any strong opinion on schema() vs. getSchema() so I'm fine with renaming it here in the name of progress and to unblock further work on entity storage :)

- static FieldItemInterface::schema($field_definition)
- FieldDefinitionInterface::getSchema()

I think static methods just defining something are ok without a "get" prefix, else I think we started doing the get prefix pretty much everywhere.

We also have Entity::baseFieldDefintions() - so that would be inline here.

FieldItem::getPropertyDefinitions() is a bit special as it is the definition + the public getter at the same time, but that's subject to with #2002134: Move TypedData metadata introspection from data objects to definition objects & co.

Thus, something along the lines of the following would make sense to me:
If it's used by devs to get something, it should have a get prefix - if it's used by devs to define something and not directly invoked, it's fine without.

ad #43: Looks good, thanks.

Missed a few.

It seems that @fago prefers to not add 'get' for this method, so #43 is the patch to RTBC :)

This no longer applies. Sorry it took me a while to get to it.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EmailItem.php
@@ -43,6 +44,39 @@ public function getPropertyDefinitions() {
+          'maxMessage' => t('%name: the e-mail address can not be longer than @max characters.', array('%name' => $this->getFieldDefinition()->getLabel(), '@max' => EMAIL_MAX_LENGTH)),

We should add $this->t() to the base class in a follow-up no? This is in the original code that was moved so shouldn't hold up commit.

@catch: That message will go away when we have a fix for #2023465: Allow validation constraints to add in message replacements. It's only there for the $name prefix so that we know which field the error message is for as form api displays them so badly.

The email property of the email item already contains complete e-mail validation, this just checks a single thing (length) and is therefore pretty stupid. That's why we had to change one of the user tests because there are now currently two validation messages in that case (the form will only display one)

Rerolled and added a schema for MapItem.

Verified that there are no other changes in there, back to RTBC.

It would help to have a better issue summary. For those of us not working on the Entity API every day, it's not clear why we are doing this, or why it is major. A better issue summary would help with the reviews.

Feel free to set back to RTBC once issue summary is done.

Wrote an issue summary and rerolled the patch. This is major because it blocks #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions, which is a beta blocker/critical on its own.

Messed up rebase :/

This is major because it blocks #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions

Actually, that makes this a critical beta blocker then.

Issue summary looks great.

I was about to RTBC this per #59, but since I haven't previously looked at this patch, I decided to review it. It all looks good, except I stumbled on #29.7. Reading the following comments about that, here's what I came up with to get that info into the patch itself.

If this is ok, please RTBC.

The migration team fully supports this patch and would love to use it. Currently we write the identifier schemas into our migrations because there's no way the sql idmap plugin could figure out how to store an 'integer' or a 'string' and if this goes in, it could!

Patch looks great - one question before committing.

+++ b/core/modules/system/system.module
@@ -66,6 +66,18 @@
+const EMAIL_MAX_LENGTH = 254;

+++ b/core/modules/user/user.module
@@ -25,11 +25,6 @@
 /**
- * Maximum length of user e-mail text field.
- */
-const EMAIL_MAX_LENGTH = 255;

Hmmm... I understand that the current email field has a max length of 254 but can we really drop the length by 1 here. What are we going to do about those users with emails of 255 characters on d6 and d7? I know this is an edge case - but it is something we should consider. And yes this probably is an existing issue.

can we really drop the length by 1 here. What are we going to do about those users with emails of 255 characters on d6 and d7?

The 'email' column from user_schema() is defined with a length of 254 (at least in D7, didn't check D6), so we're good :)

Committed bbed679 and pushed to 8.x. Thanks!

The change notice what this allows us to do should be covered by the change notice for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

users table mail column in D6 is varchar(64), so it's fine.

BTW. this is 100% broken #2174509: Drupal\Core\Field\FieldDefinition::getSchema() is broken