Turn Ubercart Addresses address into an entity

Here is my initial attempt of integrating Ubercart Addresses with Entity API. It's not perfect yet. The entity class used for the uc_addresses entity is called UcAddressesEntity and is in fact a wrapper around UcAddressesAddress. Ideally, UcAddressesAddress should be the entity class, but there were some problems I did not found an answer for so far:

• UcAddressesAddress can't extend Entity from Entity API, because the constructor methods differ.
• With the current implementation, Entity API calls methods from Entity class that UcAddressesAddress doesn't have.
• Modules expect they can access entity properties from an entity directly, but UcAddressesAddress protects it's properties, so it has more control about what values the properties will get. For example: address names are unique per address book. Protecting the address name property ensures that two addresses within the same address book never will get the same name (note: does not count for empty names).
• Entity API wants to load addresses from the database itself, while in Ubercart Addresses current architecture this should in fact be done by UcAddressesAddressBook.

The problems with the current implementation so far are:

• Entity hooks will only be invoked when working with addresses using Entity API functions, not when using the Address book API.
• Same counts for Rules events for the uc_addresses entity, automatically created by the Entity API module.
• Maybe some small issues with entity tokens, though I tried to work around the problem a bit.

The next step would be creating tests for the current implementation to reveal if there are more things that don't quite work (I've already made a start with this, see the patch). After that, I will try a different approach to see if I can fix the problems note above (and eventually new problems).

If I don't come up with a better solution soon, I might choose to stick with the current implementation for the time being, so it won't unnecessary hold up other issues that depend on this one. After these issues are fixed (and a new alpha version is released), I intend to get back to this if a different implementation is still worth the effort by that time.

Okay, somehow the Entity integration code causes the checkout tests to fail. Can it be it's incompatible with the uc_addresses_test module? I will investigate that later.

The reason the checkout test failed is because the token module assumes that if a token type is equal to a known entity type the given data object is the entity, which in this specific case isn't true. When asking for tokens of type 'uc_addresses', the given data object is an instance of UcAddressesAddress and this is not the class of the address entity in the patch (the declared address entity class is UcAddressesEntity, but this may change later).

The ideal fix for this, would be defining UcAddressesAddress as the entity class. But, like I said before, that takes some time to figure out. So, I will fix this with a workaround for the time being: implement the method uri() in UcAddressesAddress. When I manage to let UcAddressesAddress be the entity class, we probably need that method there anyway.

I've implemented tests for Entity integration for Ubercart Addresses and this revealed at least one other problem: the UcAddressesPermissions class can only check access for the current logged in user. This limitation conflicts with entity_access(), with which it possible to pass an account object to check access for.
Thus, in order to support entity_access(), a API change will be needed in UcAddressesPermissions. A change in this class can lead to unexpected results in modules that implement one of the following hooks:

• hook_uc_addresses_may_view()
• hook_uc_addresses_may_edit()
• hook_uc_addresses_may_delete()

I guess I have no choice, but to introduce an API change if I don't want to let entity_access() lead to unexpected results.
I'll open a new issue for this.

Attached is a patch that adds the automated tests for entity integration and adds a workaround for the issue with the token module. Note that the test "Ubercart Addresses entity tests" will fail because of the limitations in UcAddressesPermissions. I'm setting the status to "needs review" anyway, to check if the current implementation breaks any other tests too.

#4: uc_addresses-entity-integration-1831424-4.patch queued for re-testing.

I've opened a new issue for the UcAddressesPermissions issue:
#1894858: API change: Let UcAddressesPermissions be able to check access for other users

Note: the patch in #4 didn't execute the entity tests yet because I forgot to mention the file that test was in, in the .info file. I hope to post a new patch later this week.

Thanks, I'm now trying to let UcAddressesAddress be the entity class. It looks very promising at the moment, but is not ready yet to be posted as a patch.

This patch uses UcAddressesAddress as the entity class. The methods that were defined previously in the UcAddressesEntity are moved: the magic methods __get(), __set() and __isset() are moved to the UcAddressesSchemaAddress class. The rest of the methods that were still relevant are moved to the UcAddressesAddress class.

Also added in the tests are checks to see if entity hooks are invoked. These tests are not yet completed, because the checks are only done after calling an entity API function, not when interacting with the address book API.

The test "Ubercart Addresses entity tests" should still fail, because the API change for UcAddressesPermissions is not yet committed (see #1894858: API change: Let UcAddressesPermissions be able to check access for other users).

Let's see if this implementation breaks any other tests too.

Forgot one little piece: the method getAddress() (previously part of UcAddressesEntity) no longer exists, but the test "Ubercart Addresses entity tests" was still trying to call that.

This patch should fix at least some of the failures in the checkout test. The reason these tests failed, was because an UcAddressesAddress instance winded up in the variable table and that caused the instance to be unserialized early in the bootstrap phase. During that phase, the module functions do not exists. UcAddressesSchemaAddress calls module functions to find out which address fields there are. This could be a design flaw: perhaps classes should not depend on module functions.
I've now worked around this by checking if the specific module function exists and throw an UcAddressesUndefinedFunctionException if it doesn't. Ideally, this type of exception should be ignored in the __wakeup() method, but because then values won't be restored, I had to ignore the exception in the magic methods __get(), __set() and __isset(). It would be better if these methods weren't needed, but the Entity API expects that every entity property can be accessed directly.

The entity tests will still fail because I haven't found out yet to fire the entity load hook when addresses are loaded via the address book API. That hook is invoked differently than the hooks hook_entity_presave(), hook_entity_insert(), hook_entity_update() and hook_entity_delete().
Calling $controller->attachLoad() from UcAddressesAddressBook is not possible, because that method is protected. Maybe something like $controller->load() would do it.

The next step would be:

• Ensure that the entity load hook is invoked when an address is loaded via the address book API.

After that and if all tests pass, I think entity integration is ready. I was thinking about testing Rules integration too (comes automatically with the Entity API module), but that could as well be done in a follow-up.

I've managed to let the entity load hook be invoked when an address is loaded via the address book API. This is done by adding a public method invokeLoad() to the entity controller class that calls the protected method attachLoad(). The method is called by UcAddressesAddressBook, in which the code for loading addresses is implemented. While this is not the most neatest solution (in fact, it makes a protected method public, by providing a wrapper method for it), it is the easiest and shortest I can think of currently. But hey, UcAddressesAddressBook should be king here! The address book API has done enough to please mr. Entity API, like providing magic methods in UcAddressesSchemaAddress, just because Entity API wants to access properties directly.

If this patch passes all tests, I think it's ready for commitment. Eventually new issues about the implementation can then be handled in follow-ups.

In addition of the patch in #16, this patch removes uc_addresses tokens generated by the Entity API module. The Entity API generated the following tokens:

• uc_addresses:first-name
• uc_addresses:last-name
• uc_addresses:postal-code
• uc_addresses:address-name
• uc_addresses:default-shipping
• uc_addresses:default-billing

Ubercart Addresses has already similar tokens using an underscore instead of an hyphen. Having two tokens for the same data leads to confusion, thus that's why I just undo this action from Entity API.

I have committed #17, with one small change: I removed the method query() from UcAddressesEntityController. That method was left over from an earlier try. Phew, it was about time. I took me three months to get this thingy right.

There were some API changes in the code, or, better said, API additions, and those should be backported to the 6.x-2.x version for consistency. Specific Entity API code should not be backported, because there is no Entity API for Drupal 6.

The attached patch backports the API changes made in the classes that are part of the address book API. Invoking of entity hooks are left out of course.

Let's see if the 6.x-2.x version can handle this change immediately (passing tests) or if there are more changes needed (failing tests).

Committed #19.

Setting version back to the one the issue was originally reported for and mark this issue as fixed.