While I am aware that there was never even an alpha version of the 1.x branch, let alone a stable version, I still think that an upgrade path would be useful for sites running the 1.x branch. Looking at the changes in the 2.x branch, I don't see any requirements other than creating the commerce_addressbook_defaults table.

Comments

BassistJimmyJam’s picture

Status: Active » Needs review
StatusFileSize
new1.22 KB

Attached a patch that should take care of it (unless I'm missing something).

BassistJimmyJam’s picture

StatusFileSize
new713 bytes

Accidentally changed file mode and included some other changes in that patch that did not belong. New one is attached.

BassistJimmyJam’s picture

StatusFileSize
new1.15 KB

I didn't realize the saved profile field type had been removed. The attached patch adds removing all commerce address book saved profile fields to the update hook. This also resolved the "Undefined index: commerce_addressbook_saved_profiles" notices.

damien_vancouver’s picture

+1 on the patch from #3.

Error messages are gone, so is the old field, and the commerce_addressbook_defaults table created successfully! Thanks so much!

This also solves #1502120: Undefined index: commerce_addressbook_saved_profiles, so I marked it as a duplicate of this issue.

summit’s picture

Hi, +1 to commit patch from #3. Thanks.
greetings, Martijn

bojanz’s picture

Status: Needs review » Closed (duplicate)
+  foreach (field_info_fields() as $field) {
+    if ('commerce_addressbook_saved_profiles' == $field['type']) {
+      field_delete_field($field['field_name']);
+    }
+  }

This can't work, because at that point the field type no longer exists (it was in Addressbook 1.x and you've replaced those files with Addressbook 2.x).

The issue that you actually want is #1311470: Provide missing uninstall code.
That will allow you to uninstall Addressbook 1.x, which will delete the fields.
Then, when you install 2.x, the defaults table will be created, and all will be well.
The flow needs to be like that (uninstall -> update -> install).

BassistJimmyJam’s picture

Since the field type name is stored in the database with the field type definition, you can still get a list of all fields and look for that type. While hook_field_info is no longer available for the field type, this did not seem to introduce any issues. Perhaps the field type definition was still cached which could potentially present a problem if the cache is cleared before the update hook is run. I could change the update hook to use db_select() since all I really need is the field name. Deleting the field does not call any field type specific hooks so there is no need to be concerned about the field type no longer existing at that point.

I don't see any reason to force people to go though the rigmarole of disable -> uninstall -> update -> install when such a simple update hook could be introduced to accomplish the same task. Especially when that update hook has seen success from not only myself, but other community members as well.

bojanz’s picture

Here's some background information for you: #943772: field_delete_field() and others fail for inactive fields.
So yes, it works, but the field is still there with all its data, it has just been marked as inactive so it's not shown. Spending 5 minutes more (on reinstalling) to ensure there's no garbage data left in the database seems like a fair tradeoff to me.

BassistJimmyJam’s picture

Thanks for pointing me that issue. It looks like the issue was resolved in 7.8 and my testing was on 7.12 so I did not experience this behavior. If this were a perfect world I would still argue for an upgrade path, but it's not and I can understand being apprehensive about including a change that's dependent on core version. I will leave this to your discretion.

bojanz’s picture

Status: Closed (duplicate) » Needs work

It is resolved if you don't provide a field type AND create instances of that type in the same module, which is exactly what Addressbook did.
So because of that you can't disable Addressbook 1.x in the first place
However, I was wrong in thinking that's a problem, since you can just replace the files with 2.x ones without disabling the module.

So let's give it a shot anyway.
Here's what needs to be done:
1) Only do the update if the defaults table doesn't exist already (so that it doesn't run for existing 7.x-2.x users)
2) Remove the field_purge call, it's useless.
3) Replace the foreach (field_info_fields() as $field) { loop with a call to commerce_delete_fields('commerce_addressbook_saved_profiles');
4) See if it works.

Can you do a reroll?

BassistJimmyJam’s picture

Assigned: Unassigned » BassistJimmyJam

Absolutely, I'll get to work on it right away.

BassistJimmyJam’s picture

Assigned: BassistJimmyJam » Unassigned
Status: Needs work » Needs review
StatusFileSize
new1.04 KB

Patch attached with changes based on #10.

fakingfantastic’s picture

+1 on #12 patch. I was getting Call to undefined function commerce_addressbook_field_is_empty(). Applied patch the .install and updated the db. All cleaned up.

bojanz’s picture

Status: Needs review » Fixed

Added information to the README and committed the patch. Thank you!

Status: Fixed » Closed (fixed)

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