Field exceptions should return the name of the field that has exceptions

This should follow catch's patch in #792716: More useful field exception message. Changed priority to minor to match as well.

Since that issue is committed and closed, perhaps this one should be changed to fix all Field exceptions instead of just one or two including FieldUpdateForbiddenException (and FieldValidationException?). Tests will also need to be updated.

Here is a patch that fixes more of the useless exception messages. More messages are now run through t(), don't know if this is wanted. Also promoting the issue to "normal", as this is really an annoyance if you deal with many fields and have to guess which one is broken.

Let's see if and where the test cases need to be corrected.

+1 to this. This should make troubleshooting a lot less painful.

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Update per #3

Tagging for reroll.

Rerolled #4. The changes to list.module were removed. Its FieldUpdateForbiddenException debug message was changed in #1294264: FieldUpdateForbiddenException: Cannot update a list field - better debug info and subsequently migrated to options.module.

#6: field-exceptions-1231710-6.patch queued for re-testing.

Updating tags

#6: field-exceptions-1231710-6.patch queued for re-testing.

#6: field-exceptions-1231710-6.patch queued for re-testing.

Rerolled #6.

Most of the exceptions are covered for now, but a still couple are still remaning. Note that we use format_string in exceptions, never t().

Checked for not covered exceptions and tried to make reasonable changes.

+++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
@@ -465,7 +465,7 @@ public function getSchema() {
+        throw new FieldException(format_string('Illegal field type @field_type columns @columns.', array('@field_type' => $this->type, '@columns' => $schema['columns'])));

This will probably not work as columns is an array - field_name should be good enough I think.

Okay, I did the suggested change like discussed in IRC, thx! Thats great :)

3 line patch, is useful and still applies 1 jan 2014.

Yes these are very annoying when you hit them.

Committed/pushed to 8.x, thanks!

Should be backported.

Backported patch in #16 to 7.x.

Fixed errors for the patch in #19.

I know that in Drupal 8 format_string() is the correct function to use, but I am not sure if switching to using format_string() in Drupal 7 is appropriate.

+++ b/modules/field/field.crud.inc
@@ -60,11 +60,12 @@ function field_create_field($field) {
@@ -477,7 +478,7 @@ function field_create_instance($instance) {

@@ -477,7 +478,7 @@ function field_create_instance($instance) {
   }
   // Check that the required properties exists.
   if (empty($instance['entity_type'])) {
-    throw new FieldException(t('Attempt to create an instance of field @field_name without an entity type.', array('@field_name' => $instance['field_name'])));
+    throw new FieldException(format_string('Attempt to create a field @field_name with no entity_type.', array('@field_name' => $field['field_name'])));
   }
   if (empty($instance['bundle'])) {
     throw new FieldException(t('Attempt to create an instance of field @field_name without a bundle.', array('@field_name' => $instance['field_name'])));

The other FieldException in this code snippet still uses the translate function, t().

Should the patch maintain consistency with Drupal 7 or Drupal 8?

Attaching patch that uses translate function t(), instead of format_string() to maintain consistency with D7 code.

Patch looks ready to back port to me.

@@ -477,7 +477,7 @@ function field_create_instance($instance) {
   }
   // Check that the required properties exists.
   if (empty($instance['entity_type'])) {
-    throw new FieldException(t('Attempt to create an instance of field @field_name without an entity type.', array('@field_name' => $instance['field_name'])));
+    throw new FieldException(t('Attempt to create a field @field_name with no entity_type.', array('@field_name' => $field['field_name'])));

I don't understand this string change; field_create_instance() creates instances (not fields), so the original one is correct, right?

Regarding the use of t(), I think the current codebase is inconsistent with regard to whether it's used in exception messages (and I'm not sure there is always a right answer). So for this issue I would suggest leaving each as is so as not to rock the boat and risk introducing any strange bugs with the language system + error handling, etc....

So basically, if it uses t() already continue to use it; if it doesn't use t() already but needs placeholders, use format_string().

- Don't recall why I made that string change. Original one surely looks correct. Reverted back to original.

- Used format_string() since originally it wasn't using t() function. Though IMO, use of t() shouldn't break anything because other exception messaged are already using it.

Let me know if we can change it back to t().

exceptions should never be translated so there is no reason to use t()

well we can't set a patch to rtbc that is failing on tests. probably needs a reroll is my guess but haven't time to check it.

I tested the patch in #27 locally with the test "Search simplify" as it was failing on live. It worked fine for me.

27: drupal-1231710-field_exceptions-27.patch queued for re-testing.

ok so if it works locally let's just do a retest so we know if it is working or not.

per #30

Committed to 7.x - thanks!