Right now when you run a config import and have a storage conflict you get an error that says

"Cannot change the field type for an existing field storage."

While this might be clear when working on one field at a time, it is not specific enough when working with many fields. I propose changing the error message to say something like

"The field storage forcurrent_field_name is already set to current_field_type. Cannot change the field type for an existing field storage"

to make development easier.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rloos289 created an issue. See original summary.

rloos289’s picture

jhedstrom’s picture

This looks great, and will definitely provide better feedback.

Just some tiny nitpicks:

+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -369,7 +369,7 @@ protected function preSaveUpdated(EntityStorageInterface $storage) {
+      throw new FieldException("The field storage for " . $this->original->getName() . " is already set to " . $this->original->getType() . ". Cannot change the field type for an existing field storage");

This should use single quotes around the quoted text. The last sentence should end in a . too.

(I'm guessing this text is referenced somewhere in a test too, so that will need updating once the testbot tells us where it is :)

rloos289’s picture

rloos289’s picture

Status: Active » Needs review
Issue tags: +DX (Developer Experience)
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This makes sense to me. It would greatly improve the ability to debug a bad config import where a storage type has changes somehow.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -369,7 +369,7 @@ protected function preSaveUpdated(EntityStorageInterface $storage) {
           throw new FieldException("Cannot change the entity type for an existing field storage.");
    

    If we improving one of the messages let's improve both.

  2. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -369,7 +369,7 @@ protected function preSaveUpdated(EntityStorageInterface $storage) {
    +      throw new FieldException('The field storage for ' . $this->original->getName() . ' is already set to ' . $this->original->getType() . '. Cannot change the field type for an existing field storage.');
    

    Let's use sprintf() here. And I think we should put the important part of the message first ie. the error. And I think we should use the id() and not the name. So something like:

    throw new FieldException(sprintf('Cannot change the field type for an existing field storage. The field storage %s has the type %s.', $this->id(), $this->original->getType()));
    
alexpott’s picture

Also we can test the message in \Drupal\Tests\field\Kernel\FieldStorageCrudTest::testUpdateFieldType()

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

rloos289’s picture

New patch incorporating the suggested changes and adding in testing.

diff --git a/core/modules/field/src/Entity/FieldStorageConfig.php b/core/modules/field/src/Entity/FieldStorageConfig.php
index 91c619087b..fcfa7caf20 100644
--- a/core/modules/field/src/Entity/FieldStorageConfig.php
+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -367,10 +367,10 @@ protected function preSaveUpdated(EntityStorageInterface $storage) {
 
     // Some updates are always disallowed.
     if ($this->getType() != $this->original->getType()) {
-      throw new FieldException("Cannot change the field type for an existing field storage.");
+      throw new FieldException(sprintf('Cannot change the field type for an existing field storage. The field storage %s has the type %s.', $this->id(), $this->original->getType()));
     }
     if ($this->getTargetEntityTypeId() != $this->original->getTargetEntityTypeId()) {
-      throw new FieldException("Cannot change the entity type for an existing field storage.");
+      throw new FieldException(sprintf('Cannot change the entity type for an existing field storage. The field storage %s has the type %s.', $this->id(), $this->original->getTargetEntityTypeId()));
     }
 
     // See if any module forbids the update by throwing an exception. This
diff --git a/core/modules/field/tests/src/Kernel/FieldStorageCrudTest.php b/core/modules/field/tests/src/Kernel/FieldStorageCrudTest.php
index 98cc95581d..fe2514fd2c 100644
--- a/core/modules/field/tests/src/Kernel/FieldStorageCrudTest.php
+++ b/core/modules/field/tests/src/Kernel/FieldStorageCrudTest.php
@@ -383,6 +383,42 @@ public function testUpdateFieldType() {
     }
   }
 
+  /**
+   * Test changing a field storage type.
+   */
+  public function testUpdateEntityType() {
+    $field_storage = FieldStorageConfig::create([
+      'field_name' => 'field_type',
+      'entity_type' => 'entity_test',
+      'type' => 'decimal',
+    ]);
+    $field_storage->save();
+
+    $this->expectException(FieldException::class);
+    $this->expectExceptionMessage('Cannot change the field type for an existing field storage. The field storage entity_test.field_type has the type decimal.');
+
+    $field_storage->set('type', 'foobar');
+    $field_storage->save();
+  }
+
+  /**
+   * Test changing a field storage entity type.
+   */
+  public function testUpdateEntityTargetType() {
+    $field_storage = FieldStorageConfig::create([
+      'field_name' => 'field_type',
+      'entity_type' => 'entity_test',
+      'type' => 'decimal',
+    ]);
+    $field_storage->save();
+
+    $this->expectException(FieldException::class);
+    $this->expectExceptionMessage('Cannot change the entity type for an existing field storage. The field storage foobar.field_type has the type entity_test.');
+
+    $field_storage->set('entity_type', 'foobar');
+    $field_storage->save();
+  }

rloos289’s picture

Status: Needs work » Needs review
rloos289’s picture

FileSize
2.74 KB
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think the patch in #10 addresses everything in #7.

alexpott’s picture

Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Committed and pushed acb5680268 to 9.0.x and c176cfadea to 8.9.x. Thanks!

  • alexpott committed acb5680 on 9.0.x
    Issue #3050264 by rloos289, jhedstrom, alexpott: Better error message...

  • alexpott committed c176cfa on 8.9.x
    Issue #3050264 by rloos289, jhedstrom, alexpott: Better error message...

Status: Fixed » Closed (fixed)

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