Add CRUD API for countries [#1214492]

I'm trying to configure Ubercart in an installation profile.

One of the steps I'd like to do is to remove Canada and USA from the list of countries, so I added this to the install profile:

  // Remove Canada.
  $country_id = 124;
  $form_state = array('values' => array('country_id' => $country_id));
  drupal_execute('uc_country_remove_form', $form_state, $country_id);

There's no API function for this task, so drupal_execute seemed the logical choice, however this created a second issue.

Here's the form-submit handler for uc_country_remove_form:

function uc_country_remove_form_submit($form, &$form_state) {
  $country_id = $form_state['values']['country_id'];

  $result = db_query("SELECT * FROM {uc_countries} WHERE country_id = %d", $country_id);
  if (!($country = db_fetch_object($result))) {
    drupal_set_message(t('Attempted to remove an invalid country.'), 'error');
    drupal_goto('admin/store/settings/countries/edit');
  }

  db_query("DELETE FROM {uc_countries} WHERE country_id = %d", $country_id);
  db_query("DELETE FROM {uc_zones} WHERE zone_country_id = %d", $country_id);
  variable_del('uc_address_format_'. $country_id);

  $func_base = _country_import_include($country_id, $country->version);
  if ($func_base !== FALSE) {
    $func = $func_base .'_uninstall';
    if (function_exists($func)) {
      $func();
    }
  }

  drupal_set_message(t('!country removed.', array('!country' => $country->country_name)));
  drupal_goto('admin/store/settings/countries/edit');
}

That final drupal_goto stops any further tasks in the install profile, which means it can't be used...which means having to copy-paste the code from the submit-handler, which is against the principals of DRY.

An alternative approach would be to split the work into a separate API function:

function uc_country_remove_form_submit($form, &$form_state) {
  $country_id = $form_state['values']['country_id'];

  $result = db_query("SELECT * FROM {uc_countries} WHERE country_id = %d", $country_id);
  if (!($country = db_fetch_object($result))) {
    drupal_set_message(t('Attempted to remove an invalid country.'), 'error');
    drupal_goto('admin/store/settings/countries/edit');
  }

  uc_country_remove_country($country_id);

  drupal_set_message(t('!country removed.', array('!country' => $country->country_name)));
  drupal_goto('admin/store/settings/countries/edit');
}

/**
 * Remove a country.
 *
 * @param Int $country_id
 * Country-code according to ISO 3166-1 numeric.
 */
function uc_country_remove_country($country_id) {
  db_query("DELETE FROM {uc_countries} WHERE country_id = %d", $country_id);
  db_query("DELETE FROM {uc_zones} WHERE zone_country_id = %d", $country_id);
  variable_del('uc_address_format_'. $country_id);

  $func_base = _country_import_include($country_id, $country->version);
  if ($func_base !== FALSE) {
    $func = $func_base .'_uninstall';
    if (function_exists($func)) {
      $func();
    }
  }
}

This provides an API function that install profiles and other modules can use, and makes it easier to develop tests.

Comment	File	Size	Author
#18	country-cmi.patch	746.99 KB	longwave
#18

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

TR CreditAttribution: TR commented 10 July 2011 at 18:56

Sounds good. Can you write a patch that puts the add country, remove country, update country, and delete country functionality into separate API functions, then change the rest of Ubercart to use those functions? I'd be happy to review and commit such a patch.

Comment #2

TR CreditAttribution: TR commented 10 July 2011 at 19:01

Title:

Provide APIs, not form-submit handlers

» Add CRUD API for countries

Changing title.

Comment #3

TR CreditAttribution: TR commented 11 July 2011 at 06:07

Category:

task

» feature

Really more of a feature request.

Comment #4

TR CreditAttribution: TR commented 2 August 2011 at 02:58

@manarth: Are you going to work on this?

Comment #5

splash112 CreditAttribution: splash112 commented 9 September 2011 at 08:44

Hi,

Looks like something I would like to see happen and use.
Especially when it includes a hook on the read function to make fine-tuning of countries/regions possible.

Any chance that a non invasive patch doing this would make it in core?

Thanks
Mark

Comment #6

TR CreditAttribution: TR commented 9 September 2011 at 18:27

@splash112: See #1.

Comment #7

TR CreditAttribution: TR commented 25 September 2011 at 18:16

Version:

6.x-2.x-dev

» 7.x-3.x-dev

New features should go into 7.x-3.x first.

Comment #8

splash112 CreditAttribution: splash112 commented 23 March 2012 at 08:51

I gues best way for Drupal 7 to go on this issue is making countries a separate entity, or use the country_api somewhere else on Drupal.org.

A simple hook in the uc_country_select() function would enable people to manipulate countries selectable by the customer.

Thx
Mark

Comment #9

longwave

he/him

English

CreditAttribution: longwave commented 28 March 2012 at 12:53

Do we even need a CRUD API? It's not like the countries change often; perhaps just a hook/alter hook combination would work for both countries and zones, now we are fairly confident we have almost all correct data?

Comment #10

splash112 CreditAttribution: splash112 commented 28 March 2012 at 15:00

Can do a lot with those, yes. Probably all.

For me, I would like to be able to show only specific countries for a role. Or show a country based on the site language. Could do this by hacking away in the form the options I don't wan't or add a one row hook in the appropriate function. The second option just seems to be so much cleaner.

Comment #11

TR CreditAttribution: TR commented 28 March 2012 at 19:57

The larger issue, which is what I'm mostly interested in, is having a general API for countries, which would include but not be limited to CRUD functionality. Right now, pretty much all uses of countries throughout Ubercart involves directly querying the countries and zones tables. Likewise for contributed modules. I would like to disentangle the country code from the rest of Ubercart so that the use and manipulation of countries occurs *only* through the API. This is required if we ever want to try to fix or improve the country code - right now it's a mess that touches way too many things. We'd be a lot better off if all the country/zone/address functionality were self-contained. I'd like to leave this issue open as a reminder and as a way to document tasks that we'd like the community to contribute to.

Comment #12

splash112 CreditAttribution: splash112 commented 2 April 2012 at 15:34

There is a countries_api now that also includes (most of) the regions supported by Ubercart. For Drupal 7 there is the country module. Would it be worth the effort to copy these modules to Ubercart or create a dependency?

Comment #13

longwave

he/him

English

CreditAttribution: longwave commented 2 April 2012 at 15:39

We shouldn't create a new dependency now 7.x-3.0 is out, as it would mean existing sites that upgrade to later version of 7.x-3.x will have to install a new module.

Comment #14

splash112 CreditAttribution: splash112 commented 2 April 2012 at 16:06

Long time ago there was talk about a quick and dirty upgrade to Drupal 7.x (Ubercart 3.x) with a successor in the from op UC 4.x tackling all the architectural problems, fields, etc skipped for the sake of speed.

But that has changed now? Even big api changes are feasible in 3.x?

Comment #15

longwave

he/him

English

CreditAttribution: longwave commented 2 April 2012 at 16:23

There is no real roadmap for a 4.x branch yet, I am not opposed to opening one eventually, but perhaps not until the 6.x-2.x branch starts decreasing in usage. In the meantime I think we should fix up as much as possible in 3.x without breaking backward compatibility; it's hard enough to maintain both 2.x and 3.x at the moment without adding a third branch to manage.

As noted in #11 the first steps for this issue should be to replace the existing code that deals with countries with a consistent API rather than direct database access, we can do this in the 3.x branch without breaking existing code. We should just bear in mind that eventually we might want to replace such an API either directly with a contrib module or with a wrapper around a contrib module.

Comment #16

longwave

he/him

English

CreditAttribution: longwave commented 21 December 2012 at 17:30

Status:

Active

» Postponed

I think we can neatly solve this in 8.x with CMI, but there isn't much point changing this in 7.x now.

Comment #17

TR CreditAttribution: TR commented 13 December 2013 at 04:28

Version:	7.x-3.x-dev	» 8.x-4.x-dev
Assigned:	Unassigned	» TR
Issue summary:	View changes
Status:	Postponed	» Active

I'm working on this and other country-related issues for 8.x-4.x

Comment #18

longwave

he/him

English

CreditAttribution: longwave commented 19 March 2014 at 18:57

Status:

Active

» Needs review

File	Size
country-cmi.patch	746.99 KB

@TR: Apologies if this has duplicated work with you, but I've had this branch on the backburner for a while, it seems a reasonable approach to me and is passing basic tests - any feedback is welcome.