Move standard_country_list() to Drupal\Core\Locale\Country

Moved into utility class. Wasn't quite sure what to do with country_get_list(), so I left it in. It's only called in a couple of places, but I didn't want to replace those calls with \Drupal\Utility\Country::getStandardList(), as country_get_list() has a call to drupal_alter().

How do we now implement alter hook type functionality?

/**
 * Get list of all predefined and custom countries.
 *
 * @return
 *   An array of all country code => country name pairs.
 */
function country_get_list() {
  $countries = Country::getStandardList();
  // Allow other modules to modify the country list.
  drupal_alter('countries', $countries);
  return $countries;
}

This is already looking very good!

On one hand I think we could simply leave this as is, since it's already removing the offending includes in favor of an autoloaded class. If we want to go the full route, I guess we'll need to register Country as a DIC service (see core.services.yml), inject the module_handler service into it, and introduce a non-static Country::getList() method that would leverage the MH service to alter the value returned by Country::getStandardList(), which should become non-static too.

The end usage would look something like this:

<?php
// Inject the $country service in OO code or retrieve it from the DIC in procedural code:
$this->country->getlist();
// or
Drupal::service('country')->getList()
?>

Like this? New ground for me :)

+++ b/core/lib/Drupal/Core/Utility/Country.php
@@ -0,0 +1,289 @@
+    static $countries;

One more thing: this should become a protected class variable. If we make Country a service we'll a singleton instance, hence it would be ok to have a non-static variable, which would be more flexible to deal with.

Move $countries to \Drupal\Core\Utility\Country::countries.

Interdiff for above.

Sorry, #5 was a crosspost. Didn't see #4 yet, nice work :)

Some more fixes and then we should be ready to go:

+++ b/core/core.services.yml
@@ -424,3 +424,6 @@ services:
+  country:
+    class: Drupal\Core\Utility\Country

Probably for consistency with other services we should rename this to country_manager and CountryManager respectively.

+++ b/core/includes/install.core.inc
@@ -2349,7 +2349,7 @@ function _install_configure_form($form, &$form_state, &$install_state) {
+  $countries = \Drupal\Core\Utility\Country::getStandardList();

Isn't the DIC available, here? Drupal::service('country_manager') ...

+++ b/core/lib/Drupal/Core/Utility/Country.php
@@ -0,0 +1,321 @@
+class Country {

This should implement a CountryManagerInterface. See the documentation of the Drupal class for more details on dependecy injection.

+++ b/core/lib/Drupal/Core/Utility/Country.php
@@ -0,0 +1,321 @@
+  public static function getStandardList() {

This should be non static so we can swap the actual implementation.

+++ b/core/modules/system/lib/Drupal/system/Form/RegionalForm.php
@@ -25,7 +26,7 @@ public function getFormID() {
+    $countries = \Drupal::service('country')->getList();

It would be better to inject the country manager when instantiating the RegionalForm object instead of directly retrieving it from the DIC, if there is a way to do it.

+++ b/core/scripts/update-iso-3166.sh
@@ -34,8 +34,8 @@
+$existing_countries = Country::getStandardList();

I think it's ok to directly instantiate the CountryManager class here.

Moved \Drupal\Core\Utility\Country to Drupal\Core\Locale\CountryManager, and created a CountryManagerInterface.

Hopefully this is all ok. I think update-iso-3166.sh might be a bit ropey, but it *seems* to work.

+++ b/core/includes/install.core.inc
@@ -2349,7 +2349,7 @@ function _install_configure_form($form, &$form_state, &$install_state) {
+  $countries = Drupal::service('country_manager')->getStandardList();

This should be ->getList().

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -0,0 +1,324 @@
+  static protected $countries;

As I was saying above I don't think we should statically cache this, as we'd have no way to reset cache. A non-static class field should be fine since a service object is only instantiated once, normally.

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -0,0 +1,324 @@
+      $t = get_t();
+
+      $countries = array(

This is not properly indented.

+++ b/core/modules/system/lib/Drupal/system/Form/RegionalForm.php
@@ -8,6 +8,7 @@
+use Drupal\Core\Utility\Country;

+++ b/core/modules/system/system.module
@@ -10,6 +10,7 @@
+use Drupal\Core\Utility\Country;

These are obviously unneeded :)

+++ b/core/modules/system/system.api.php
@@ -3476,7 +3476,7 @@ function hook_updater_info_alter(&$updaters) {
+ * @see \Drupal\Core\Utility\Language::standardCountryList()

I think this should mention \Drupal\Core\Locale\CountryManager::getList() now.

Thanks :)

Awesome! Just a couple of minor things and we are done:

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -0,0 +1,324 @@
+      $this->countries = $this->getStandardList();

I think that under certain PHP configurations this would cause an E_STRICT warning to be thrown: static::getStandardList() should be safer.

+++ b/core/modules/system/system.module
@@ -10,6 +10,7 @@
+use Drupal\Core\Utility\Country;

One stale "use" left from the previous review :)

+++ b/core/modules/system/system.api.php
@@ -3476,7 +3476,7 @@ function hook_updater_info_alter(&$updaters) {
+ * @see \Drupal\Core\Locale\Country::getList().
  */

Ehm, this should be \Drupal\Core\Locale\CountryManager::getList()

@cam8001:

Are you still working on this?

Yes! Planning to put aside some time tonight UTC. If I'm holding anything up feel free to push it through, but otherwise I'd like to finish it up.

Cool, thanks!

Rebased and tweaked.

There are still a couple of lingering wrong @see directives :(

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -0,0 +1,324 @@
+   * @see \Drupal\Core\Utility\Country::getStandardList()

Should be \Drupal\Core\Locale\CountryManager::getStandardList().

+++ b/core/modules/system/system.api.php
@@ -3501,7 +3501,7 @@ function hook_updater_info_alter(&$updaters) {
+ * @see \Drupal\Core\Locale\Country::getList().

Should be CountryManager::.

Damn, sorry. Should be clean now.

This is starting to get embarassing ;)

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -0,0 +1,324 @@
+ * Definition of Core\Utility\CountryManager.

Core\Utility > \Drupal\Core\Locale

+++ b/core/lib/Drupal/Core/Locale/CountryManager.php
@@ -0,0 +1,324 @@
+   * @see \Drupal\Core\Utility\CountryManager::getStandardList()

Utility > Locale

Mate, you're telling me!

I noticed there doesn't seem to be a consistent naming standard for @see directives or "Definition of" docblocks. Some are \Drupal\Namespace\Of\Class and others are Drupal\Namespace\Of\Class. I've gone with the first option, as I think it is less ambiguous. It also seems to be codified here: https://drupal.org/node/1354#see

Awesome!

Just a fresh reroll of #20. Looks good to me now :)

@cam8001:

If you wish to keep working on this stuff, there's #1845034: Convert standard.inc to Drupal\Core\Language that needs some more love :)

Reroll of #21.

RTBC again if the bot is happy

+++ b/core/modules/system/lib/Drupal/system/Form/RegionalForm.phpundefined
@@ -25,7 +25,7 @@ public function getFormID() {
-    $countries = country_get_list();
+    $countries = \Drupal::service('country_manager')->getList();

We can inject the country manager by overriding create() and __construct from SystemConfigFormBase in RegionalForm.

Ah, right, I had noted that somewhere above, forgot about it.

Injected the country manager as suggested in #25.

#27: 2004506-standard_country_list-27.patch queued for re-testing.

Missed some use statements.

RTBC if green :)

Committed 07e3ccb and pushed to 8.x. Thanks!

And here it is :)

https://drupal.org/node/2019329