Datetime type should be disabled on storage setting form if field has data

Run Testbot, run!

We need an integration test to make sure we don't regress this later.

Raising to Major this bug can lead to data loss.

I'll update IS from the template when I get time later, and we need a short beta eval.

Tests for #2.

+++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
@@ -12,6 +12,8 @@
 use Drupal\Core\Entity\Entity\EntityViewDisplay;
 use Drupal\Core\Datetime\DrupalDateTime;
 use Drupal\simpletest\WebTestBase;
+use Drupal\field\Entity\FieldStorageConfig;
+use Drupal\field\Entity\FieldConfig;

Ideally, we would have these additions in alphabetical order.

Patch doesn't apply, so looks like this needs a reroll.

Updated IS and rerolled patch with a small clean-ups

Go!

1. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
   @@ -9,12 +9,12 @@
    use Drupal\Component\Utility\SafeMarkup;
    use Drupal\Component\Utility\Unicode;
   -use Drupal\Core\Entity\Entity\EntityViewDisplay;
    use Drupal\Core\Datetime\DrupalDateTime;
   +use Drupal\Core\Entity\Entity\EntityViewDisplay;
    use Drupal\field\Entity\FieldConfig;
   +use Drupal\field\Entity\FieldStorageConfig;
    use Drupal\node\Entity\Node;
    use Drupal\simpletest\WebTestBase;
   -use Drupal\field\Entity\FieldStorageConfig;
    
   

   My original comment about alphabetical order for `use` was based on the patch I was reviewing at the time, which just had two additions. It looks like that was a bad diff, so this change may be considered OOS for the issue at hand.

2. +++ b/core/modules/datetime/src/Tests/DateTimeFieldTest.php
   @@ -805,6 +805,48 @@ function testInvalidField() {
   +    $result = $this->xpath("//*[@id='edit-settings-datetime-type' and contains(@disabled, 'disabled')]");
   +    $this->assertEqual(count($result), 1, "Changing datetime setting is disabled.");
   +    $this->assertText('There is data for this field in the database. The field settings can no longer be changed.');
   +  }
   

   Like the fact that we are not just checking for the error message on the page.

I'm not going to change the status back, but catch or alex may push this back for the `use` reordering.

I tested this manually, and am fine with the patch. Seriously, it's a one liner. However, what I find really confusing now is that when you disable the field storage settings b/c they have data, you see that error message but you can change the cardinality of the field. In 7, this was in a different place, and had a slightly better error message.

This is global to fields in 8; not sure if we have an issue about this.

@alexpott, @effulgentsia, and I discussed that this might even be critical because of the data loss (and therefore well worth considering a small form change in a patch release).

Committed 10e9709 and pushed to 8.1.x. Thanks!

This is rc eligible for 8.1.x as it is a critical bug. I consider upgrade implications for sites which have changed this setting - as far as I can see there is nothing we can do so going ahead with commit.