Provide more graceful handling of exceptions during field base updates [#2360373]

As many of you I'm sure have encountered, if a field definition changes on an existing field that has data, a FieldUpdateForbiddenException will be thrown, which kills the entire reverting process, and fails to give you any meaningful information about which feature and field are causing the issue. This leaves the developer to either bisect through features and fields, or to hack up Features module to print it out in features.field.inc. I propose we instead log these to the watchdog log.

Comment	File	Size	Author
#4	features-2360373-field-reverting-exceptions-4.patch	4.13 KB	q0rban
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

q0rban CreditAttribution: q0rban commented 20 October 2014 at 22:50

Title:

Provide more graceful handling of exceptions during field updates

» Use watchdog_exception() in features.field.inc when encountering update exceptions.

Ah, nevermind, I see this is already fixed in the dev version of Features, however, we should switch to using watchdog_exception(). Updating title.

Comment #2

q0rban CreditAttribution: q0rban commented 20 October 2014 at 22:58

Title:

Use watchdog_exception() in features.field.inc when encountering update exceptions.

» Provide more graceful handling of exceptions during field base updates

I take it back again, looks like this is only happening for instances, not for the fields themselves.

Comment #3

q0rban CreditAttribution: q0rban commented 20 October 2014 at 23:07

And, also, doesn't show the name of the feature module that has the exported field.

Comment #4

q0rban CreditAttribution: q0rban commented 20 October 2014 at 23:15

Status:

Active

» Needs review

File	Size
features-2360373-field-reverting-exceptions-4.patch	4.13 KB

Comment #5

deviantintegral CreditAttribution: deviantintegral commented 21 October 2014 at 13:16

I really like getting a bit more detail into the error messages. I'm not sure about discarding exceptions we don't handle. Especially since watchdog messages don't show up by default in drush, it seems like it's really easy to miss.

What about adding more detail into the exception itself, but we still throw it? That way calling code can choose to handle it if it really can. Exceptions are final so we can't just change the message, but we could catch the existing exception and then throw a new one with the updated message.

Comment #6

q0rban CreditAttribution: q0rban commented 21 October 2014 at 13:50

Especially since watchdog messages don't show up by default in drush, it seems like it's really easy to miss.

Watchdog messages do indeed show up in drush. Screenshot of what drush output looks like with this patch.

I considered still throwing the exception as well, but seeing as how an exception is not thrown when field_instance update/create fails, I opted to follow that pattern. I am open to throwing the exception if that is deemed best.

Comment #7

deviantintegral CreditAttribution: deviantintegral commented 22 October 2014 at 00:32

Doh, the level is error, not warning.

IMO we should throw the exception in update and create as well, given that it's implying that something is horrifically wrong. I usually find that once one field starts failing that everything else does too due to dependencies.

Comment #8

q0rban CreditAttribution: q0rban commented 22 October 2014 at 13:07

I'm okay with making that change, but for the sake of playing devil's advocate, and assuming you are a developer that pays attention to errors in the deployment process, can you describe a situation where having the process continue could be catastrophic? The reason I ask is because in my experience when this occurs, something is NOT horrifically wrong—it's just that the field length has changed. Everything will work perfectly fine as is, but you should either get the field definition to match the database, or write an update hook to clean up your data and alter the field schema by hand.