Addresses from orders are not stored in UC Addressbook

Thanks for filing a bug report, especially one with useful details provided! This bug probably got in due to recent changes, as addresses were saved in the address book after completing checkout before. Not sure if you were implying this in step 1, but addresses entered by an admin when editing an order (admin/store/orders/%uc_order/edit) are not automatically saved to the address book, but it should save addresses automatically to the address book when completing checkout.

I hope to look at this issue soon as possible, but there is a lot of other work for me left to be done.
If you would like to help with coding, you could write an automated test for this case. This helps to make the code more stable and prevents the issue for coming back (after it's been solved). Do this only if you are familiar with writing tests, as it takes some time to learn how to write tests.

reworded

Thanks for your efforts to work on this! I don't think the changes you propose in your patch from #3 is the ideal way of fixing this issue. You make changes to the behavior of the API to fix a specific case. The compareAddress()-method should do what it suggests to do: check if the given address looks like any of the other addresses in the address book, saved or not. Also, by adding global $user; the API method will be unusable if addresses are compared for an user that's not logged in.

I like the approach of your first patch better (from #2), that implies to do a compare only against the addresses that are saved.

There is a better way of checking if an address is saved or not. Instead of checking if the address contains a "created" value or a negative address ID, you can call the method isNew() on an UcAddressesAddress object:

/**
 * Checks if the address is a temporary unsaved
 * address
 *
 * The address is new if it has negative ID
 *
 * @access public
 * @return boolean
 */
public function isNew() {
  return ($this->getId() < 1);
}

By the way, if you post a patch for review, the right issue status to set it to, is "needs review". This way, the patch will go through the automated tests process (if supported by the project) which helps to reveal if the modules get's broken elsewhere.
See also this article about automated patch testing: http://drupal.org/node/1058140
"patch (to be ported)" is used when a patch has been committed for one version, but needs to be adopted for another version. See also this article about the issue status: http://drupal.org/node/156119.

Well, I will first try to reproduce your issue. With me, addresses are saved to the address book with the default settings. I will going to try it with your settings.

Setting to "needs work", because the issue should be fixed in an other way.

Hm, unfortunately, I'm not able to reproduce your issue so far. I've tried to reproduce the issue several times under these circumstances (not all at once):

• With a minimal amount of modules enabled.
• With a minimal amount of modules enabled plus Extra Fields Pane, Flat Rate, Payment, Payment method pack, Shipping quotes and Taxes.
• With the Ubercart default settings.
• With the delivery pane enabled and disabled.
• With shipping quotes and taxes configured.

I've used the settings you provided and followed these steps:

1. Disabled the delivery checkout pane in the checkout settings.
2. Not logged in, act as an anonymous user.
3. Added one product to the cart (/cart).
4. Went to checkout (/cart/checkout).
5. Filled in the billing address.
6. Went to checkout review (/cart/checkout/review).
7. Submitted the order by clicking the "Submit Order" button.
8. Got login details from Ubercart. Logged in as this user.
9. Went to the address book page (user/8/addresses). There was one address created.

I repeated the steps above as a logged in user with no previous orders and no addresses in the address book. Thus with step 2 replaced with this:
- Logged in as an existing user (that has no orders or addresses).
I got the same result.

Used versions

• Ubercart Addresses 7.x-1.x-dev (latest)
• Ubercart 7.x-3.2
• Drupal 7.15
• PHP 5.3.2

While I think that the bug is in Ubercart Addresses, I'm not able yet to reproduce it. Maybe the bug only comes to life when Ubercart Addresses is used in combination with an other module?

Could you try either one of these?:

• Disable all modules except the four "required" Ubercart core modules, Ubercart Addresses and all the modules that Ubercart Addresses needs to function.
• On a fresh install, only enable the four "required" Ubercart core modules, Ubercart Addresses and all the modules that Ubercart Addresses needs to function.

And try if you encounter the same issue.

If you do get the same issue, then try to explain the steps you took in more detail (inclusive Ubercart checkout configurations).
If you do not get the same issue, then it's likely the bug only exists when Ubercart Addresses is used in combination with an other module. The trick is then to find out which one. The first modules to look for are the modules that influence the checkout process.

My bad, I noticed I had been testing with a version that already included a fix for #1735678: Autofill doesn't work, which "hided" the problem. I've been able to reproduce the issue now. The issue occurs when the address forms at checkout are not prefilled with the default addresses. This is an option that can be turned on or off, but there had been bug where the address forms where never got prefilled, no matter what option was set (see #1735678: Autofill doesn't work for details).

I've been busy debugging today and I found that the root of the problem lies much deeper: at some point during PHP runtime there are two address objects with the same address ID, but different PHP object ID's. That's why UcAddressesAddressBook compares the two addresses, it only skips addresses in comparison that are 100% equal (having the exact same PHP object ID):

if ($address === $addressBookAddress) {
  // We don't need to compare the address with itself
  continue;
}

So the solution (or at least a part of the solution) would be to prevent having two address objects with the same address ID. How is this possible in the first place? Well, when unserializing a PHP object, it get's a new PHP object ID assigned. I think the solution would be to check if an address object with same ID already exists when an address object get's unserialized. If this is the case, then overwrite the "new" unserialized address object with the one that's already available.

I have the following code so far in the UcAddressesAddress class:

/**
 * Restore variables when the address is unserialized.
 *
 * @access public
 * @return void
 */
public function __wakeup() {
  parent::__wakeup();
  $this->addressBook = UcAddressesAddressBook::get($this->getSchemaAddress()->uid);
  $aid = $this->getId();
  // Check if we already have an address with the same ID in the address book.
  if ($this->addressBook->addressExists($aid)) {
    // We do not allow having two address objects with the same address ID
    // during runtime, so we overwrite the unserialized object with the
    // address object that's already available.
    // PHP does not allow to overwrite $this directly, but we can cheat
    // by passing $this to a static method an overwrite the object there.
    $address2 = $this->addressBook->getAddressById($aid);
    self::reassignThis($this, $address2);
    return;
  }
  if ($aid <= self::$nextNewAid) {
    self::$nextNewAid = $this->getId() - 1;
  }
  try {
    $this->addressBook->addAddress($this);
  }
  catch (UcAddressesException $e) {
    // Ignore any exceptions.
  }
}

/**
 * Helper function to "re-assign this".
 *
 * This method is here to make sure we never end up with two
 * addresses with the same ID.
 * If an address is unserialized with an ID that is already
 * available in the address book, the unserialized address
 * will be overwritten with the current available address.
 *
 * This method should only be called by the __wakeup() method.
 *
 * @param UcAddressesAddress $address
 *   The address that was unserialized.
 * @param UcAddressesAddress $address2
 *   The address that already was available in the address book,
 *   having the exact same ID.
 *
 * @access private
 * @return void
 * @throw UcAddressesInvalidParameterException
 */
private static function reassignThis(UcAddressesAddress $address, UcAddressesAddress $address2) {
  if ($address->getId() != $address2->getId()) {
    throw new UcAddressesInvalidParameterException(t('Tried to overwrite an address with an address that got an other ID.'));
  }
  $address = $address2;
}

I'm working on it.

My ideas posted in #6 didn't work at all. I can't find a way to overwrite an address object immediately after it's being unserialized. The only way I see for preventing having two address objects with the same ID at runtime is disallowing serializing. As that would have a huge impact on the existing code, I decide to not fix that weak spot (I might as well rewrite the whole module, which I'm not going to do if I look at the amount of my free time).

Instead, I've worked on an acceptable workaround that fixes this issue. A change is made in the UcAddressesAddressBook class so that addresses are only compared to saved addresses. I've written some automated tests to ensure saving addresses to address book will work with or without the autofill option on. This revealed some other small issues, such as:

• In some cases when doing a checkout with no addresses in the address book, saved addresses were only marked as default shipping and not default billing (and vice versa).
• Filling in two equal addresses at checkout could result into two addresses being created (this was after the change in UcAddressesAddressBook was made).
• When editing addresses in the address book (user/%uid/addresses/%uc_addresses_address/edit), a comparison was not done (and thus you could create double addresses). I seem to have forgotten to include that check before.

So for one fix several other small fixes were also needed in order to let all automated tests pass.

I've made the change in both 6.x-2.x and 7.x-1.x:

• 6.x-2.x commit
• 7.x-1.x commit