Problem/Motivation

If you set a a date value on a date field of an entity using
\Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::setDateTime()
and then try to save the entity you will receive a fatal error:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22001]: String data, right truncated: 1406 Data too long...

That is because DateTimeIso8601::setDateTime() uses PHP's c date format which is of the form 2004-02-12T15:19:21+00:00. DateTimeItem::schema() intends to save dates without the timezone offset suffix, so it enforces a maxlength of 20.

Even if we increase the maxlength and allow saving dates in the c format in the database you cannot actually use them, because DateTimeComputed does not function with dates in that format either.

Long story short: The typed-data-level handling of dates and the field-level handling of dates both work fine on their own, but not when used together. As the latter builds on the former that is a problem as can be seen above.

Proposed resolution

Change the storage format to explicitly store a time zone offset value of '+00:00' in the field value. This matches the STORAGE_TIMEZONE. Using the long form for the offset, instead of the 'Z' shorthand, is used to future proof the storage length in case the offset gets changed in the future to allow any valid +HH:MM value.

Remaining tasks

- update path for daterange
- test with base fields
- figure out test coverage for field definitions?

User interface changes

None.

API changes

DATETIME_DATETIME_STORAGE_FORMAT and DateTimeItemInterface::DATETIME_STORAGE_FORMAT are different.

Data model changes

Field values are stored with an explicit time zone offset of '+00:00' per the updated storage formats.

CommentFileSizeAuthor
#84 interdiff-81-84.txt3.51 KBmpdonadio
#84 2716891-84.patch178.75 KBmpdonadio
#81 interdiff-79-81.txt12.34 KBmpdonadio
#81 2716891-81.patch178.61 KBmpdonadio
#79 interdiff-77-79.txt4.99 KBmpdonadio
#79 2716891-79.patch178.1 KBmpdonadio
#77 interdiff-76-77.txt7.96 KBmpdonadio
#77 2716891-77.patch35.24 KBmpdonadio
#76 interdiff-73-76.txt1.1 KBmpdonadio
#76 2716891-76.patch31.77 KBmpdonadio
#73 interdiff-70-73.txt3.89 KBmpdonadio
#73 2716891-73.patch31.56 KBmpdonadio
#72 Screen Shot 2018-01-31 at 3.31.30 PM.png229.62 KBjhedstrom
#70 interdiff-68-70.txt7.22 KBmpdonadio
#70 2716891-70.patch31.08 KBmpdonadio
#68 interdiff-63-68.txt4.28 KBmpdonadio
#68 2716891-68.patch187.11 KBmpdonadio
#63 interdiff-57-63.txt7.22 KBmpdonadio
#63 2716891-63.patch29.04 KBmpdonadio
#57 interdiff-55-57.txt8.11 KBmpdonadio
#57 2716891-57.patch29.83 KBmpdonadio
#55 interdiff-51-55.txt15.46 KBmpdonadio
#55 2716891-55.patch21.72 KBmpdonadio
#51 interdiff-48-51.txt4.91 KBmpdonadio
#51 2716891-51.patch6.26 KBmpdonadio
#49 interdiff-48-49.txt5.09 KBmpdonadio
#49 2716891-49.patch6.88 KBmpdonadio
#48 interdiff-46-48.txt628 bytesmpdonadio
#48 2716891-48.patch3.61 KBmpdonadio
#46 2716891-46.patch3 KBmpdonadio
#40 2716891-40.patch2.96 KBmpdonadio
#30 rerolling-2716891-30.patch12.4 KBhardik.p
#24 interdiff-22-24.txt1.17 KBmpdonadio
#24 2716891-24.patch12.27 KBmpdonadio
#22 interdiff-21-22.txt4.7 KBmpdonadio
#22 2716891-22.patch11.09 KBmpdonadio
#20 interdiff-2716891-14-21.txt8.11 KBSonal.Sangale
#20 2716891-21.patch8.27 KBSonal.Sangale
#14 2716891-14.patch8.11 KBmpdonadio
#12 interdiff-10-12.txt2.59 KBmpdonadio
#12 2716891-12.patch8.1 KBmpdonadio
#10 2716891-10.patch5.52 KBmpdonadio
#4 2716891-04.patch3.68 KBmpdonadio
#2 2716891-test-only.patch1.91 KBmpdonadio
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tstoeckler created an issue. See original summary.

mpdonadio’s picture

Status: Active » Needs review
FileSize
1.91 KB

The PHP 'c' date format (used in DateTimeIso8601) outputs with the +HH:MM format for time zone specification. The test coverage uses the Z shorthand. Minimal test showing *at least* two ways this causes problems. I think this is limited to DateTimeItemTest based on some quick searches in PhpStorm, but I am not totally sure how deep this problem reaches.

Gotta think about this one... Painful as this patch will be b/c update path, I think that

diff --git a/core/modules/datetime/datetime.module b/core/modules/datetime/datetime.module
index 8682399..bb1b9e2 100644
--- a/core/modules/datetime/datetime.module
+++ b/core/modules/datetime/datetime.module
@@ -15,7 +15,7 @@
 /**
  * Defines the format that date and time should be stored in.
  */
-const DATETIME_DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s';
+const DATETIME_DATETIME_STORAGE_FORMAT = 'c';
 
 /**
  * Defines the format that dates should be stored in.

and related changes to normalize core to the 'c' format may be the proper thing to do here so that all of the parts match each other.

mpdonadio’s picture

Title: DateTimeIso8601::setDateTime() vs. DateItem::schema(): (at least) one of them is broken » DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken
Issue summary: View changes
mpdonadio’s picture

Curious what breaks with this.

The last submitted patch, 2: 2716891-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2716891-04.patch, failed testing.

tstoeckler’s picture

Yay, this patch looks awesome, thanks!!!! Yeah, BC/upgrade path is not going to be fun.

One additional problem is what to do if people set their date fields to DATETIME_DATE_STORAGE_FORMAT. Then the saving will work because the length is shorter than that of DATETIME_DATETIME_STORAGE_FORMAT but the loading will not work.

mpdonadio’s picture

tstoeckler’s picture

Status: Postponed » Needs work
mpdonadio’s picture

Status: Needs work » Needs review
FileSize
5.52 KB

Status: Needs review » Needs work

The last submitted patch, 10: 2716891-10.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs upgrade path, +Needs upgrade path tests
FileSize
8.1 KB
2.59 KB

OK, fixed the test that triggered the child issue. Stubbed out update hook. Need to dig into other fails, and see if the presence of the update hook triggers more fails.

Status: Needs review » Needs work

The last submitted patch, 12: 2716891-12.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
8.11 KB

Reroll b/c patch didn't apply + renamed the update hook for 8.2.x.

Status: Needs review » Needs work

The last submitted patch, 14: 2716891-14.patch, failed testing.

jhedstrom’s picture

  1. +++ b/core/modules/datetime/datetime.install
    @@ -0,0 +1,25 @@
    +  // @todo Figure out how to do the above.
    

    Something like this perhaps?

    \Drupal::service('entity_type.manager')->getStorage('field_storage_config')->loadByProperties(['type' => 'datetime', 'settings' => ['datetime_type' => 'datetime']]);
    

    Alternatively, just load all type = 'datetime' field storages and loop through in code to filter out date-only storages.

  2. +++ b/core/modules/datetime/datetime.module
    @@ -14,8 +14,11 @@
    +const DATETIME_DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:sP';
    

    Over in #2632040: Add ability to select a timezone for datetime field it was tentatively decided that storage should always be in UTC. If we add the P here, doesn't that start storing local times instead?

mpdonadio’s picture

#16-2, yes, you are totally correct. That should probably be

+const DATETIME_DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s+00:00';
mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Unassigning myself if anyone wants to work on this at DevDays.

Sonal.Sangale’s picture

Assigned: Unassigned » Sonal.Sangale
Sonal.Sangale’s picture

Status: Needs work » Needs review
FileSize
8.27 KB
8.11 KB

Given it a try!

Please review!

Status: Needs review » Needs work

The last submitted patch, 20: 2716891-21.patch, failed testing.

mpdonadio’s picture

Assigned: Sonal.Sangale » Unassigned
Status: Needs work » Needs review
FileSize
11.09 KB
4.7 KB

I think this upgrade path works. This is lifted heavily from something @Berdir did on another patch. He should really get credit here.

But, reading values out of the database is now borked. They end up in the database OK, but don't get read out properly. I suspect it has to do with the change to DATETIME_DATETIME_STORAGE_FORMAT, but I need to play with that more.

Oh, and this upgrade path is still in progress. Running it against a real site is a bad idea. It may destroy your database. Test against a test site.

Status: Needs review » Needs work

The last submitted patch, 22: 2716891-22.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
12.27 KB
1.17 KB

This is ugly, but works with manual testing.

After looking at usage reports in PhpStorm, I am worried that we are using DATETIME_DATETIME_STORAGE_FORMAT and DATETIME_DATE_STORAGE_FORMAT in contexts unrelated to storage, and just as generic format strings.

Status: Needs review » Needs work

The last submitted patch, 24: 2716891-24.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 24: 2716891-24.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs reroll
hardik.p’s picture

Assigned: Unassigned » hardik.p
hardik.p’s picture

Assigned: hardik.p » Unassigned
Status: Needs work » Needs review
FileSize
12.4 KB

Patch Attached.

Status: Needs review » Needs work

The last submitted patch, 30: rerolling-2716891-30.patch, failed testing.

The last submitted patch, 30: rerolling-2716891-30.patch, failed testing.

mpdonadio’s picture

I think we need to see if the daterange code needs to be touched by this.

And I think the fails also mean that we need to update the test fixture as well as do the update path test.

mpdonadio’s picture

Wondering if we need to reverse this, and update DateTimeIso8601 to match storage first, to get everything consistent in core (and not upgrade path!), and then as a followup rework storage for ISO 8601 format w/ +00:00 timezone.

?

Manuel Garcia’s picture

Issue tags: -Needs reroll

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

benjy’s picture

I just ran into this, I was using Carbon like so: Carbon::now()->subDays(10)->toIso8601String() to set a date field in a test, but Carbon's version of ISO8601 (which is correct) causes the same fatal because the schema isn't long enough.

Dane Powell’s picture

Note that the way storage works right now is in violation of ISO 8601, as discussed here: #2914779: Date field values are stored without timezone identifier

So I would prefer that storage gets updated to use a timezone identifier of some sort.

mpdonadio’s picture

Start of a do-over. No tests, and no upgrade path. Mainly want to see what fails, esp since I don't think we addressed any misuses of the storage formats as general date formats.

Status: Needs review » Needs work

The last submitted patch, 40: 2716891-40.patch, failed testing. View results

Wim Leers’s picture

Related: #2926508-24: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON API:

It looks like all fields using the DateTimeIso8601 @DataType are happy to allow RFC3339 timestamps, (for example 2017-03-01T20:02:00-00:00, the -00:00 at the end is allowed by RFC3339 but not by ISO8601), it's only the DB layer that's preventing this from happening. IOW: \Drupal\datetime\Plugin\Validation\Constraint\DateTimeFormatConstraintValidator::validate() is not strict enough yet.

mpdonadio’s picture

Priority: Normal » Major

I am tentatively raising this to a major. I think it may be the root cause of some DX weirdness we are encountering on other issues.

jhedstrom’s picture

#40 makes sense. Should we work on the update hooks and tests now?

+++ b/core/modules/datetime/src/DateTimeComputed.php
@@ -46,7 +46,15 @@ public function getValue($langcode = NULL) {
+    if ($item->getFieldDefinition()->getSetting('datetime_type') === 'date') {

This should probably use the constant instead of 'date'.

Wim Leers’s picture

#44++

Let's get this moving forward again!

mpdonadio’s picture

Status: Needs review » Needs work

The last submitted patch, 46: 2716891-46.patch, failed testing. View results

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
628 bytes

Bad reroll.

mpdonadio’s picture

This should be more better.

The last submitted patch, 48: 2716891-48.patch, failed testing. View results

mpdonadio’s picture

Found a hunk that can go.

The last submitted patch, 49: 2716891-49.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 51: 2716891-51.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
21.72 KB
15.46 KB

No upgrade path yet, and the update test fail, but the rest of the tests should be fixed.

Status: Needs review » Needs work

The last submitted patch, 55: 2716891-55.patch, failed testing. View results

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
29.83 KB
8.11 KB

This is totally untested. Mostly copied from #24, but needs EntityManager swapped out for the proper services.

I'm wondering if we need to add datetime_range to the standard fixture?

Status: Needs review » Needs work

The last submitted patch, 57: 2716891-57.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

I tested the datetime update manually, and it looked like it worked. Need to check the datarange one.

At a loss as to how to fix these fails:

datetime module
Update #8601
Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal86.node__field_test_3' doesn't exist: SELECT * FROM node__field_test_3; Array ( ) in datetime_update_8601() (line 58 of /Users/matt/Documents/PhpStorm/drupal-8.6.x/core/modules/datetime/datetime.install).

I am assuming a change is needed to drupal-8.filled.standard.php, but I am not sure what really is needed.

Berdir’s picture

+++ b/core/modules/datetime_range/datetime_range.install
@@ -0,0 +1,106 @@
+      if ($changes['revision_table']) {
+        $revision_table = $changes['revision_table'] . '__' . $field_name;
+        $schema->changeField($revision_table, $value_field_name, $value_field_name, $field_spec);
+
+        $results = \Drupal::database()->query('SELECT * FROM ' . $revision_table)->fetchAll();
+        foreach ($results as &$result) {
+          Drupal::database()
+            ->update($revision_table)
+            ->fields([
+              $value_field_name => $result->{$value_field_name} . '+00:00',
+              $end_value_field_name => $result->{$value_field_name} . '+00:00',
+            ])
+            ->condition('bundle', $result->bundle)

we have date fields with many 100k's of entries, there is no way you can load them all and process in a single update function in a loop :)

If all you do is add "+00:00" then I don't understand why you don't just do an update function that does that inline in the database with update ... set field = concat(field, '+00:00')?

Berdir’s picture

But even then, we should imho do this in a batch because this will take a long time even when done as a single update query. So first collect all the tables and then do one per call or so.

mpdonadio’s picture

From @berdir in Slack:

@mpdonadio nope, the fixture is fine, your code is wrong :slightly_smiling_face:

[5:37 PM]
@mpdonadio because it doesn't consider table prefixes
[5:37 PM]
you're missing {} around the tables
[5:38 PM]
but that should actually solve itself when you switch to the update with concat() as we discussed
[5:38 PM]
because ->update() does that for you

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
29.04 KB
7.22 KB

Will convert this to a batch operation once this basically works, but not sure what is missing for this fail (AggregatorUpdateTest and others):

Update save_custom_storage_property
Failed: Drupal\Core\Entity\Exception\FieldStorageDefinitionUpdateForbiddenException: The SQL storage cannot change the schema for an existing field (field_test_3 in node entity) with data. in Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->updateDedicatedTableSchema() (line 1506 of /Users/matt/Documents/PhpStorm/drupal-8.6.x/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).

Status: Needs review » Needs work

The last submitted patch, 63: 2716891-63.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jhedstrom’s picture

@mpdonadio that error will occur if the entity schema definitions aren't completely up to date after all update hooks have run, so I'm guessing it indicates the new update hook here isn't completely updating the field/entity schema data...

Berdir’s picture

Yeah, I think it's not doing that at all. It's not enough to just update the schema in 8.x, you also need to tell the entity field definition manager that you did update it which is annoyingly complicated.

jhedstrom’s picture

I think any update hook using \Drupal::entityDefinitionUpdateManager() can probably be referred to for an example. However, IIRC, actual changes to the table schema can be a bit trickier, but I think some of those exist in core already.

A nit on the current patch:

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -227,7 +227,7 @@ protected function buildDate(DrupalDateTime $date) {
    +    $iso_date = $date->format("Y-m-d\TH:i:sP", ['timezone' => 'UTC']);
    

    This should use DateTimeItemInterface::STORAGE_TIMEZONE.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -282,7 +282,7 @@ public function testDatetimeField() {
    +            $expected_iso = format_date($date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    

    And ditto here.

  3. +++ b/core/modules/datetime_range/tests/src/Functional/DateRangeFieldTest.php
    @@ -332,10 +332,10 @@ public function testDatetimeRangeField() {
    +    $start_expected_iso = $this->dateFormatter->format($start_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    ...
    +    $end_expected_iso = $this->dateFormatter->format($end_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    
    @@ -413,7 +413,7 @@ public function testDatetimeRangeField() {
    +    $start_expected_iso = $this->dateFormatter->format($start_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    
    @@ -500,10 +500,10 @@ public function testAlldayRangeField() {
    +    $start_expected_iso = $this->dateFormatter->format($start_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    ...
    +    $end_expected_iso = $this->dateFormatter->format($end_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    
    @@ -580,10 +580,10 @@ public function testAlldayRangeField() {
    +    $start_expected_iso = $this->dateFormatter->format($start_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    ...
    +    $end_expected_iso = $this->dateFormatter->format($end_date->getTimestamp(), 'custom', 'Y-m-d\TH:i:s+00:00', 'UTC');
    

    And these while we're at it :)

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
187.11 KB
4.28 KB

Modeling this off of system_update_8007(), which doesn't appear to use \Drupal::entityDefinitionUpdateManager(), but directly messes with the K-V store. Still not sure if I need the `$manager->updateFieldStorageDefinition($manager->getFieldStorageDefinition($field_name, $entity_type));` line node_update_8002() does, but after the K-V shenanigans.

Need to add the loop through \Drupal::keyValue('entity.storage_schema.sql') and address the nits, but I can't think through this anymore tonight.

Status: Needs review » Needs work

The last submitted patch, 68: 2716891-68.patch, failed testing. View results

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
31.08 KB
7.22 KB

Nits from #67 and a rebased patch (interdiff on #63 was OK, but apparently I forgot to rebase it). This is mainly just a clean patch to look at for the status of the update hook.

Status: Needs review » Needs work

The last submitted patch, 70: 2716891-70.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jhedstrom’s picture

Issue summary: View changes
FileSize
229.62 KB

This isn't fully updating the installed schema. After some digging (follow #2747789: Improve UpdatePathTestBase debuggablity to make this easier), in SqlContentEntityStorageSchema::requiresFieldStorageSchemaChanges, the $installed_schema still has the old length for both the field and the revision field:

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
31.56 KB
3.89 KB

Need a full run, and this is still ugly, but this should fix most fails?

Status: Needs review » Needs work

The last submitted patch, 73: 2716891-73.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

Been digging into this for a while. UpdatePathRC1TestBaseFilledTest is failing during one of the multilingual tests, but can't tell what isn't happening.

mpdonadio’s picture

OK, need to only append when datetime (not dateonly). This isn't the right way to handle this, but want a full run while I am working on cleanup. I *think* this will be green.

mpdonadio’s picture

Status: Needs review » Needs work

The last submitted patch, 77: 2716891-77.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
178.1 KB
4.99 KB

Still don't get why DateTimeUpdate8601Test::testFieldDefinition() fails. I have tried a whole bunch of different ways to get the schema, and the column length is always 25 before the updates run.

Really want to get the test in a good place before adding batching to the update, and then copy/paste/update for daterange.

Status: Needs review » Needs work

The last submitted patch, 79: 2716891-79.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpdonadio’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
178.61 KB
12.34 KB

Still can't figure out DateTimeUpdate8601Test::testFieldDefinition(), so worked on batching the updates.

jhedstrom’s picture

  1. +++ b/core/modules/datetime/tests/src/Functional/Update/DateTimeUpdate8601Test.php
    @@ -0,0 +1,106 @@
    +      __DIR__ . '/../../../../../datetime/tests/fixtures/update/2716891.php.gz',
    

    Do we not have any datetime fields in the core standard install db dump?

  2. +++ b/core/modules/datetime/tests/src/Functional/Update/DateTimeUpdate8601Test.php
    @@ -0,0 +1,106 @@
    +    $schema = $field_definitions['field_datetime']->getSchema();
    +    $this->assertEquals(20, $schema['columns']['value']['length']);
    

    The reason this is failing is that getSchema() actually calls out to the DateTimeItem::schema() method, rather than the stored schema.

mpdonadio’s picture

#82-1, the standard filled fixture has a datetime field, in date-only mode (IIRC, field_test_3). The coverage checks that date+time gets updated and that date-only doesn't, so the existing one isn't sufficient. The date range fields will also need in a fixture, and I was planning on using the new one for this, too.

#82-1, o_O but I guess that kinda of makes sense. Manually grabbing from the K-V doesn't seem right (too close to the update code), but that seems like the only option?

mpdonadio’s picture

Here's what poking through the K-V would look like. Is this good enough for test? If so, I will clone to daterange and then work on tests for base fields.