Catch field exception when rebuilding

Re-rolled against 7.x-1.x and normalized patch to -p1.

Also removed t() call for watchdog, implemented similar logic for field instances, and raised watchdog severity to WATCHDOG_ERROR.

The patch from #2 worked flawlessly for me.

I'm really torn about committing this one to Features. Isn't the problem in Field Collections? Features uses a lot of different Drupal Hooks and other modules can always hook in and cause problems. I don't think trapping exceptions on every call with possible external hooks like this is really the way to handle it. Interested in other opinions.

It happened for me, but not in Field Collections.

The point is there could be any number of reasons a field fails to rebuild. A FieldUpdateForbiddenException triggered by field_sql_storage_field_update_forbid() can easily be mirrored in some other field storage provider.

I didn't include this, but since you're now aware of this issue, I was thinking of a more robust Features error reporting mechanism. Of course, that would be a new feature request to provide a more informative mechanism than (or in addition to) watchdog().

Yes.
The patch from #2 worked flawlessly for me too.
Thanks.
Already patched file is here:
http://alperbalci.com/defol_git/features.field.rar
Please extract and put this file on your server,
/sites/all/modules/features/includes
or equivalent directory.

GANYANCI: You should not be posting links to patched files. You can attach files to your post here directly.

This saved my day.

Still haven't figured out how to patch files in a windows environment.

Much appreciated.

Still happening in 2.x.

This can happen if you are working in a development environment on multiple branches and your database gets out of sync, for example, or in other situations involving modules not being enabled correctly.

I'm really torn about committing this one to Features. Isn't the problem in Field Collections? Features uses a lot of different Drupal Hooks and other modules can always hook in and cause problems. I don't think trapping exceptions on every call with possible external hooks like this is really the way to handle it. Interested in other opinions.

The root cause is that the Database API throws an Exception if the table does not exist (see #1551132: When trying to create a table that already exists but is empty, recreate the table rather than throwing a DatabaseSchemaObjectExistsException). Its up to the module implementing the call which triggers that exception to catch it. If the cause is a missing module, which it is, then it can't be the missing modules job to catch it, it must be Features.

I think the flipside to this issue is that when a feature is updated beyond previously compatible parts, logically an update hook is required to perform changes and migrate content (just like any other module).

I believe Features is meant for use only as the exporting (EDIT) and importing aspects of configuration management (prior to D8, of course). Since the artifacts being managed mostly don't understand change beyond a database update (e.g. fields), change management of Features exports is a bit out of its league. Developers will just have to do that job (i.e. in an update hook).

I dread to propose "won't fix" or "works as designed".

Although actually modifying a field without touching the Field CRUD API is tricky. The regular field_update_field() function consults SQL storage (which forbids the change) but bypassing it and giving field info to drupal_write_record() saves crap data into field config. To safely update field configuration, you'll have to db_select() and db_update() to store the modified config data into {field_config} (and obviously migrate whatever field SQL table columns to be updated).

Maybe a complementary task for Features is in order to help facilitate configuration change management for developers (on a number of exportable components).

I dread to propose "won't fix" or "works as designed".

At minimum, I would expect features not to crash an entire module save process just because of one error like this. It's essentially unrecoverable without commenting out bits of code. The exception handlers don't break other updates, at least.

Fair enough.

So, would you RTBC the patch in #2?

I tested this a bit more this afternoon in the 7.x-2.x branch...

My issue, as it turns out was that the tables for the fields existed, but for some reason were no longer in the field information. This is a slightly different problem, because the Field API CRUD handlers don't throw a FieldException for this, but let the database API throw a DatabaseSchemaObjectExistsException instead ("table exists").

Ironically, although this seems like the sort of thing Features shouldn't deal with, it comes about quite easily when you are using Features to add configuration to version control, and you have multiple feature branches.

I added extra catch blocks for the DatabaseSchemaObjectExistsException, however while this allowed me to complete the transaction, my feature was STILL overridden.

So, while I don't have a problem putting up the 2.x patch, I'm wondering whether it's going to be any help?

So, while I don't have a problem putting up the 2.x patch, I'm wondering whether it's going to be any help?

It's definitely going to help, as it gives the user detailed information on where the problem is occurring, rather than simply providing an error message without context. With this information, it's possible to look through one's content types and fields based on the new output, locate the mismatch, and then fix it.

This applies to the latest dev branch. RTBCing based on that, my code review and the above comments proving effect.

#2: 1664160-02-catch-field-exception-when-rebuilding.patch queued for re-testing.

#2 worked for me. Thanks @zhangtaihao !

Well, I spoke too soon. If I re-run a second time:

drush fra -y

I get the same errors that a field(s) already exist:

WD php: DatabaseSchemaObjectExistsException: Table field_data_field_add_discussion_file already exists. in [error]
DatabaseSchema->createTable() (line 657 of ... /includes/database/schema.inc

#1: features_catch_field_update_exception.patch queued for re-testing.

Added in commit 71f017c.

FTR, this error took my site down.
I had a content type (upgraded from D6) that had node representing user profiles (originally profile_node)
Rebuilding this functionality in D7, as a stand-alone project, we built a data schema directly only user entities instead. But no special fields, mostly just text.
We exported that feature, and tried to apply it to our target site.
The "Features" page crashed with an "Unknown error" and refused to work again.
Our "Modules" page then did the same, leaving things unstable.
I disabled the feature in drush (glad that worked) then diagnosed the issue.

Watchdog showed me

 FieldException: Cannot change an existing field's type. in field_update_field() (line 234 of modules/field/field.crud.inc). 

Which led me to find a name conflict
in our old node content type (field_position) and our new user field (field_position)

... I'll trial the Aug1 dev release and see if this prevents this sort of death, thanks.
I'm just writing here to describe one use-case for searchers. Or, maybe (I can't be quite sure) this is a similar but different issue?

.dan.

Blast, with 7.x-2.0-rc2 dated Aug2, , and todays 7.x-2.x-dev this still dies.
The website encountered an unexpected error. Please try again later.

This must be a new issue. sorry.

This issue doesn't "fix" anything related to the crash. All it does is add additional information via watchdog to the error log files. So be sure you check your watchdog error location for the additional information given on which field is causing the problem.

Anyone interested in the DatabaseSchemaObjectExistsException problem might like to look at #1923006: New feature with existing tables fails

It looks like the catch statements around the field base functions was removed in a later commit. I've attached a patch that adds the watchdog error back for the field bases and makes the formatting consistent between the two error handlers.

I also updated the original catch blocks so that they catch any exception instead of just the Field exceptions (because the database schema could be throwing an exception as well).

This got fixed over in issue #2534138: field_base_features_rebuild doesn't catch exceptions.. Probably shouldn't have re-opened such an old issue instead of creating a new one as this went un-noticed.