Comments

cyberwolf’s picture

Unfortunately the iso2 code is stored in the field instead of the integer primary key of the country entity. This seems to make integration with the Entity API more difficult...

alan d.’s picture

Cross posting a support request to the entity api queue: #1142536: How to integrate the countries module with the entity api.

alan d.’s picture

Status: Active » Needs work

Right, this should be as easy as adding a property to the entity definition and to change the load callback.

If you want to help push this through, the following two functions need updating to test.

function countries_entity_info() {
  $return = array(
    'country' => array(
      'label' => t('Country'),
      'base table' => 'countries_country',
      'fieldable' => TRUE,
      'entity keys' => array(
        'id' => 'cid',
        'name' => 'iso2',  // <<<< This is new
      ),
      'bundles' => array(
        'country' => array(
          'label' => t('Country'),
          'admin' => array(
            'path' => 'admin/config/regional/countries',
            'access arguments' => array('administer site configuration'),
          ),
        ),
      ),
      'view modes' => array(
        'full' => array(
          'label' => t('Country'),
          'custom settings' => FALSE,
        ),
      ),
    ),
  );
  return $return;
}

function countries_load($cid, $reset = FALSE) {
   // <<<< This is new
  if (!is_numeric($cid)) {
    return countries_get_country($cid);
  }
 // <<<< End of changes

  $countries = countries_load_multiple(array($cid), array(), $reset);
  return reset($countries);
}
webflo’s picture

I like, but whats with countries_load_multiple()?

alan d.’s picture

I'm not sure if this is needed or not for the integration with the Entity API. I was getting ready to start testing this out and got side-tracked with teaching my niece some math and then other projects. ;)

A quick code search of the Entity API module did not show up any calls, but testing would be required...

webflo’s picture

Status: Needs work » Needs review
StatusFileSize
new15.14 KB

I think we should leverage entity api. It makes our code more readable and we got integration in other modules for free. I have written some tests and refactored the countries module.

alan d.’s picture

Wow, you have been active! Great work. The only comments are to do with moving the two functions, country_load() and countries_country_lookup(). The first may be used as a menu wildcard loader, so this should be in the general path, and the latter is a generic bridging function that can be used to replace almost all of the country_api functions in one hit. As such, I think that these two should stay in the main module file.

alan d.’s picture

Me bad. Sorry, I didn't look close enough at the patch and missed the require_once. I think that it is best not re rename the generic country loader function still.

I've committed through three new hook calls to help to integrate this module with others, they went in with the commit for the safe_value feature request (50292c3)

For quick reference:

    module_invoke_all('country_insert', $country);
    module_invoke_all('country_delete', $country);

    // To account for a changed iso2 value, this records the old value.
    $country->original_iso2 = $iso2;
    module_invoke_all('country_update', $country);
webflo’s picture

Bullet number #2 :)

alan d.’s picture

Looking good!

Since this is such a major patch, I've combined the other minor patches in the queue so that these can also go through same testing process at the same time. I've done basic tests on most areas and I'm happy if you commit directly to DEV. The size is mainly due to the altered readme.txt.

Includes:
* Removed the Country FAPI element
* New country filter (as per CCK Country which this module replaces)
* Cleaner implementation of the filters.
* Removed hook_field_sanitize().
* Fixed hook_options_list().

Related to entity integration, I have modified a number function signatures to match the internal entity function calls but no other changes.

alan d.’s picture

StatusFileSize
new2.14 KB

Ignore this patch - unrelated

alan d.’s picture

The right one this time

johnv’s picture

Is the dependency on Entity API a necessity?
Rules module depends on Entity API, so Rules integration requires Entity API anyway.
So, for simple sites, it would be nice if Countries still works witout Entity dependency.

alan d.’s picture

For this patch yes.

There is no real way to toggle things if the entity module is present or not. But unlike CTools or one of the other common requirements, the entity api is fairly lightweight.

alan d.’s picture

It looks like a number of users are using the country element. I have readded the code for this.

@webflo
I'm happy if this is pushed through to dev. Are you?

alan d.’s picture

I was just doing more tests, namely testing that the entity module calls the hooks, I found a minor issue with the update hook (still used the original_iso2 property).

Also, the unique indexes on the ISO3, Official name, and number code will throw errors if two or more are left blank. The db unique keys have been dropped from the schema (these are still enforced in code).

alan d.’s picture

@webflo
After a couple of weeks after releasing this patch, I think that we should push out the first proper 2.x release depending on how your i18n tasks going.

alan d.’s picture

I just noticed that the admin delete country submit handler still invokes hook_country_delete(). I am assuming that the entity api handles this too? Sorry, no testing environment to see if this is getting invoked twice.

webflo’s picture

Yes you are right. Here is another patch. I refactored countries_admin_delete_submit() and added countries_delete(). I am not sure about the naming convention. The entity type is "country" so country_load, country_load_multiple etc. makes totaly sense but the "module namespace" is countries.

webflo’s picture

I uploaded the wrong patch. Sorry.

johnv’s picture

I'll try and test this soon.
I need a view with the following: EntityType A -> Countries <- EntityType C. Entity and Views support this 'reverse relationship'. I hope this patch will enable this for Countries?
(When creating a Views relationship, you'd see a 'field_country' and a 'field_country -reverse-')

webflo’s picture

Status: Needs review » Fixed

Ok. I think this patch is good. Commit 6aa27d6 on 7.x-2.x.

Status: Fixed » Closed (fixed)

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

glajman’s picture

Issue summary: View changes

@johnv Did you achieve the following : View with EntityType A -> Countries <- EntityType C functionality ?

I need something similar. I cannot see how to get the "reverse relationship" working or the "field_country_reverse" you mentioned.

Does anybody have a tip ?

-G

johnv’s picture

@glajman, you need to chain the relationships, starting with A.

glajman’s picture

@johnv thx for the quick reply.

We are not using Entity Reference to relate the entities, just the country field in Entity A and Entity C.

We tried chaining the relationships but did not get the expected results. We should only get 1 row as a result:
Country Name, Entity A field, Entity C field

Are we doing something wrong or missing something ?

Thx for your help!!

-G