Updated: Comment #60

Problem/Motivation

In Drupal 8 we introduced the concept of base fields which are "full blown" fields, but their schema is still hardcoded in hook_schema() implementations of their entity types. If we want to be able to auto-generate the entity base tables for multilingual and performance purposes (don't create revision tables if the entity type is not revisionable), we need an API to retrieve the schema for base fields.

--> This task is a blocker for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions.

More generally, we want to get rid of the separation between 'field types that can be used for base fields" & "fields types that can be used for configurable fields" - we have one unique notion of "field type", but we still have sub-interfaces for "field types that can be used for configurable fields". This is another step in that direction.

Proposed resolution

Make all field types provide a schema(), and clarify the relation between the schema and the "property definitions" and the respective ways to access them.

Remaining tasks

None.

User interface changes

None.

API changes

  • The schema definition method ConfigFieldItemInterface::schema() moves up a level to FieldItemInterface::schema(), making it a requirement for all field types (configurable + base fields)
  • Consequently, Drupal\field\FieldInterface::getSchema() moves to Drupal\Core\Field\FieldDefinitionInterface::getSchema(), making FieldDefinition the authoritative way of getting the schema for a field type
Files: 
CommentFileSizeAuthor
#66 interdiff.txt1.44 KBeffulgentsia
#66 move_schema_to_field_definition-2144327-66.patch48.05 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 59,345 pass(es).
[ View ]
#63 interdiff.txt703 bytesamateescu
#63 move_schema_to_field_definition-2144327-63.patch47.61 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,305 pass(es).
[ View ]

Comments

yched’s picture

Issue summary:View changes
amateescu’s picture

Status:Active» Needs work
Parent issue:» #2144263: Decouple entity field storage from configurable fields
StatusFileSize
new30.06 KB

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.

dasjo’s picture

StatusFileSize
new29.84 KB

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

amateescu’s picture

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 :)

dasjo’s picture

ops, sorry for the noise then :)

fago’s picture

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.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new44.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,990 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new18.06 KB

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?

yched’s picture

where to document the relationship between property names and schema columns

Maybe in the phpdoc for FieldItemInterafce::schema() ?
"The 'columns' need to be a subset of the properties defined in getPropertyDefinitions()" ?

The last submitted patch, 7: move_schema_to_field_definition-2144327-7.patch, failed testing.

amateescu’s picture

StatusFileSize
new48.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,038 pass(es).
[ View ]
new5.62 KB

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

yched’s picture

Why do you feel it could be affected by #2112239: Convert base field and property definitions ? (maybe it is, just not clear to me)

Just a couple remarks after a quick look, doesn't really qualify as a review :-)

  1. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
    @@ -36,13 +36,14 @@
    +   *     getPropertyDefinitions(). This specifies what comprises a value for a
    +   *     given field. For example, a value for a number field is simply 'value',
    +   *     while a value for a formatted text field is the combination of 'value'
    +   *     and 'format'. It is recommended to avoid having the column definitions

    The part about "This specifies what comprises a value... combination of 'value' and 'format'" is actually out of place now, we should remove that.
    Defining "what comprises a value for a field type" is done in getPropertyDefinitions().

  2. +++ b/core/modules/system/system.module
    @@ -343,7 +344,7 @@ function system_element_info() {
    -    '#maxlength' => defined('EMAIL_MAX_LENGTH') ? EMAIL_MAX_LENGTH : 255,
    +    '#maxlength' => EmailItem::EMAIL_MAX_LENGTH,

    Hm - why is this needed ? sounds weird to have a base FAPI element refer to a constant defined by the "email field type" class ?

  3. +++ b/core/lib/Drupal/Core/Field/ConfigEntityReferenceItemBase.php
    @@ -78,13 +77,13 @@ public function getPropertyDefinitions() {
    -  public static function schema(FieldInterface $field) {
    -    $definition = \Drupal::service('plugin.manager.field.field_type')->getDefinition($field->type);
    +  public static function schema(FieldDefinitionInterface $field_definition) {
    +    $definition = \Drupal::service('plugin.manager.field.field_type')->getDefinition($field_definition->type);

    Won't work, should be $field_definition->getFieldType()

amateescu’s picture

StatusFileSize
new47.24 KB
FAILED: [[SimpleTest]]: [MySQL] 58,927 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new7.74 KB

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.

yched’s picture

Not fully sure what you have in mind, but at any rate the $field_definition received by the schema() method should already be FieldDefinitionInterface objects anyway, even if not all baseFieldDefinitions() methods have not been converted from the old array syntax ? There's a BC layer that converts them to objects just out of baseFieldDefinitions().

amateescu’s picture

Then I guess I'm just tired :P

The last submitted patch, 12: move_schema_to_field_definition-2144327-12.patch, failed testing.

The last submitted patch, 12: move_schema_to_field_definition-2144327-12.patch, failed testing.

amateescu’s picture

StatusFileSize
new46.99 KB
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es).
[ View ]
new4.6 KB

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.

yched’s picture

I'm not sure I understand - how could some external code decide of 'not_null', 'default' values, if not the FieldItem::schema() ?
(although, not sure either why we'd need to provide a 'default' at all ? The current "configurable" field types that already provide a schema() in HEAD don't provide any ?)

amateescu’s picture

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 :)

yched’s picture

amateescu’s picture

StatusFileSize
new46.99 KB
FAILED: [[SimpleTest]]: [MySQL] 59,047 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolled.

fago’s picture

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?

fago’s picture

  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?

Status:Needs review» Needs work

The last submitted patch, 21: move_schema_to_field_definition-2144327-21.patch, failed testing.

The last submitted patch, 21: move_schema_to_field_definition-2144327-21.patch, failed testing.

effulgentsia’s picture

Issue tags:+beta target

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.

amateescu’s picture

Status:Needs work» Needs review
Issue tags:+LONDON_2013_DECEMBER
StatusFileSize
new47.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,285 pass(es).
[ View ]
new945 bytes

$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.

yched’s picture

WIll try to review monday.

Regarding 'default_value':

- Yes, there's the unfortunate clash with the Entity/Field API notion of "default value", that is actually unrelated. Would add confusion, and we can't really rename either one.

- We always save "complete" db records, with data for all columns (be it NULL, ''...). So a 'default value' specified in the schema will never be about "what value gets written on partial inserts".

- The remaining case is "when adding a new column to an existing table, what to put in the column for the existing rows".

This is why D7/configurable fields never had to bother with that (per field tables, we don't add new fields to a table), and the only code that did was hook_updates that added a new "column/property" to an existing field type - but that was only in updates, and the hook_update contained an explicit schema without calling hook_field_schema().

It seems even now that we're putting several (base) fields in the same table, adding new ones after the table creation will be a job for hook_update ?

So yes, I don't think I see a use case for adding 'default value' in the field schemas ?

yched’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/DateItem.php
    @@ -43,4 +44,20 @@ public function getPropertyDefinitions() {
    +          // @todo Needs a size. What kind of value do we store here anyway, a
    +          // timestamp?

    Hah - nothing in core currently uses the 'date' field type :-)
    Well, I guess the target would be to store a timestamp, yes ?

    The "configurable" DateTimeItem that was added in core from contrb date.module choses to store dates as ISO strings (2013-12-18T03:01:00), but we're probably not going to switch node.created and friends to that format - at any rate not in that issue ;-)
    So yes, I'd tend to think we go with just 'type' => 'int', as node_schema() currently does ?

    Then again, if this 'date' field type was intended for things like node.created, not sure why it currently uses an 'integer' field instead.

    @fago, does it ring any bell on your side ?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/UriItem.php
    @@ -42,4 +43,17 @@ public function getPropertyDefinitions() {
    +          'type' => 'text',

    For consistency with configurable fields (and for our current base table schemas) we should add a not_null TRUE in there - and in the other converted base fields as well. Only varchars should have not_null FALSE.

  3. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/ConfigurableEntityReferenceItem.php
    @@ -25,43 +26,21 @@ class ConfigurableEntityReferenceItem extends ConfigEntityReferenceItemBase impl
    +    $schema = EntityReferenceItem::schema($field_definition);

    Why not parent::schema() ?

  4. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
    @@ -178,13 +178,6 @@ class Field extends ConfigEntityBase implements FieldInterface {
    -   * The field schema.
    -   *
    -   * @var array
    -   */
    -  protected $schema;
    -
    -  /**

    The class still uses this, no ?

  5. +++ b/core/modules/system/system.module
    @@ -7,6 +7,7 @@
    +use Drupal\Core\Field\Plugin\Field\FieldType\EmailItem;

    Leftover :-)

  6. +++ b/core/modules/text/lib/Drupal/text/Plugin/Field/FieldType/TextItem.php
    @@ -31,12 +31,12 @@ class TextItem extends TextItemBase {
    -          'length' => $field->settings['max_length'],
    +          'length' => $field_definition->settings['max_length'],

    Oops - this should never have been ->settings['foo'], but ->getFieldSetting('foo') :-)

  7. +++ b/core/modules/user/lib/Drupal/user/Tests/UserValidationTest.php
    @@ -109,9 +110,9 @@ function testValidation() {
    -    $this->assertEqual(count($violations), 1, 'Violation found when email is too long');
    +    $this->assertEqual(count($violations), 2, 'Violation found when email is too long');

    Seems weird hat this gives two violations now.
    So it looks like the "too long" check is already performed somewhere else:

    getPropertyItems() defines the 'value' property as being of DataType Drupal\Core\TypedData\Plugin\DataType\Email, which defines a constraint using \Symfony\Component\Validator\Constraints\EmailValidator, which performs a filter_var($value, FILTER_VALIDATE_EMAIL);

    So maybe we shouldn't be adding an additional constraint for max length at the field level then ?

Berdir’s picture

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.

yched’s picture

Thanks for the explanations, Berdir :-)

re: date fields, added some considerations in #2150511-10: [meta] Deduplicate the set of available field types

amateescu’s picture

StatusFileSize
new47.05 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new6.35 KB
  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 last submitted patch, 32: move_schema_to_field_definition-2144327-32.patch, failed testing.

amateescu’s picture

StatusFileSize
new47.05 KB
FAILED: [[SimpleTest]]: [MySQL] 59,202 pass(es), 66 fail(s), and 34 exception(s).
[ View ]
new838 bytes
yched’s picture

3. Ah true. Well then:
ConfigEntityReferenceItemBase::schema() does "if (old hook_field_schema exists) {return schema from the old hook}". We could add an "else {return parent::schema()}" after that ?
Then ConfigurableEntityReferenceItem::schema() can just do "parent::schema()", and will require no further change when the "old hook" BC layer is removed ?

7. I think @Berdir's comment means we should keep the duplicate constraints / violations for now ?

yched’s picture

Although, @Berdir re email constraint / violations:
it surprises (and worries :-)) me that we're able to assign field-level violations back to the right widget, but not property-level violations. You say there is an existing issue for that ?

Berdir’s picture

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.

The last submitted patch, 34: move_schema_to_field_definition-2144327-34.patch, failed testing.

amateescu’s picture

StatusFileSize
new102.38 KB
FAILED: [[SimpleTest]]: [MySQL] 36,586 pass(es), 902 fail(s), and 695 exception(s).
[ View ]
new3.35 KB

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.

The last submitted patch, 39: move_schema_to_field_definition-2144327-39.patch, failed testing.

amateescu’s picture

StatusFileSize
new47.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,345 pass(es).
[ View ]

And the correct patch this time.

fago’s picture

+++ 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 ...

amateescu’s picture

StatusFileSize
new46.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,326 pass(es).
[ View ]
new1.71 KB

Like this?

yched’s picture

From the omment I just posted at #2116341-1: Apply defaults to definition objects:

But right, settling on a pattern of :
static FieldItem::schema($field_definition)
static FieldItem::getPropertyDefinitions($field_definition)
static FieldItem::getConstraints($field_definition)
would make sense - although, er, would that mean schema() should be getSchema() ?

What has "get", what doesn't have "get" ?

amateescu’s picture

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

yched’s picture

I'd hate to derail that issue on naming, but... yes, that the question :-)

We have:
- static FieldItemInterface::schema($field_definition)
- FieldDefinitionInterface::getSchema()
The former is the source, the latter is the API accessor which fetches from the former. IIRC, this distinction was the reason why I went with slightly different names - even though, well, those actual names don't really convey what the difference is :-/
Don't know what I had in mind exactly - the idea that a static method cannot be a getter, or maybe that having a "get" prefix hints "call me, not the other one"...

Since this might start a pattern with [#8269327] (which also carries an issue of "which is which, which one should you call"), and we're touching all those lines here for schema(), we might as see if we want to reconsider that ?

Again, i don't think we want to derail that one, just pointing similarities and patterns. Unless we have a quick consensus, we can simply say "let's think about it in [#8269327]".

amateescu’s picture

StatusFileSize
new47.38 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new19.7 KB

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 :)

The last submitted patch, 47: move_schema_to_field_definition-2144327-47.patch, failed testing.

fago’s picture

- 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.

amateescu’s picture

StatusFileSize
new48.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,308 pass(es).
[ View ]
new1.92 KB

Missed a few.

amateescu’s picture

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

yched’s picture

StatusFileSize
new46.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,380 pass(es).
[ View ]

Sorry @amateescu, I didn't intend to say "let's do it now" but rather "let's discuss a bit", I should have been clearer :-/

So yes, agreed with no get prefix for the static "initial definition" methods. and static FieldItem::getPropertyDefinitions() would be changed to comply in #2002134: Move TypedData metadata introspection from data objects to definition objects.

So yes, it looks like #43 is RTBC.
Reuploading it for clarity.

yched’s picture

Status:Needs review» Reviewed & tested by the community

Meant to do this.

catch’s picture

Status:Reviewed & tested by the community» Needs work

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.

Berdir’s picture

@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)

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new47.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,830 pass(es).
[ View ]
new988 bytes

Rerolled and added a schema for MapItem.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

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

Dries’s picture

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.

moshe weitzman’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs issue summary update

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

amateescu’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new47.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 60: move_schema_to_field_definition-2144327-60.patch, failed testing.

The last submitted patch, 60: move_schema_to_field_definition-2144327-60.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new47.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,305 pass(es).
[ View ]
new703 bytes

Messed up rebase :/

yched’s picture

Issue summary:View changes

Thanks @amateescu.
Expanded the summary a bit.

effulgentsia’s picture

Priority:Major» Critical
Issue tags:-beta target+beta blocker

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.

effulgentsia’s picture

Issue tags:-Needs issue summary update
StatusFileSize
new48.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,345 pass(es).
[ View ]
new1.44 KB

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.

yched’s picture

Status:Needs review» Reviewed & tested by the community

Yup, fine if green

chx’s picture

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!

alexpott’s picture

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.

amateescu’s picture

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 :)

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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.

penyaskito’s picture

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

chx’s picture

Status:Fixed» Closed (fixed)

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