Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 |
Comments
Comment #1
q0rban CreditAttribution: q0rban commentedAh, 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 commentedI take it back again, looks like this is only happening for instances, not for the fields themselves.
Comment #3
q0rban CreditAttribution: q0rban commentedAnd, also, doesn't show the name of the feature module that has the exported field.
Comment #4
q0rban CreditAttribution: q0rban commentedComment #5
deviantintegral CreditAttribution: deviantintegral commentedI 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 commentedWatchdog 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 commentedDoh, 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 commentedI'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.
Comment #9
Gik000 CreditAttribution: Gik000 commentedI had a huge problem because of a field corruption after the feature.
Anyway #4 worked like a charme!
Thank you so much