Views sorting by country name

I'd prefer this for the CountryCode docblock:

/**
 * Sort by country.
 *
 * By default Views will sort by country code.
 * This handler also allows sorting by country name.
 *
 * @ViewsSort("country_code")
 */

Other points:

+    $this->langcode = $language_manager->getCurrentLanguage()->getId();

Why use the content language? By default the country list will be returned in the UI (config override) language, which makes more sense?

+        $when[] = 'WHEN ' . $field_name . ' = \'' . $country_code . '\' THEN \'' . $country_name . '\'';

At minimum we'd want to run $field_name through $connection->escapeField(), no?
(You can inject $connection, since db_escape_field() is deprecated).

I'm also concerned that passing $country_code and $country_name unescaped creates a theoretical SQL injection vulnerability.
Furthermore, if a country name has an apostrophe then it will surely crash.
The first thing we can do is pass an index instead of $country_name, the sorting effect is the same, but the query is smaller and there's no chance of injection/query breakage via the real $country_name.
That leaves us with only $country_code to worry about (which is easier cause we know it's always three letters, we can filter somehow).

Great! The last remaining point from #4 is:

+    $this->langcode = $language_manager->getCurrentLanguage()->getId();

Why use the content language? By default the country list will be returned in the UI (config override) language, which makes more sense?

Looks like we never did this part:

I'm also concerned that passing $country_code and $country_name unescaped creates a theoretical SQL injection vulnerability.
Furthermore, if a country name has an apostrophe then it will surely crash.
The first thing we can do is pass an index instead of $country_name, the sorting effect is the same, but the query is smaller and there's no chance of injection/query breakage via the real $country_name.

We should also typehint the country repository using the library interface, like the widget and formatters do.

Have there been any updates on this behind the scenes? The patches no longer apply.

Yes, any news? This seems like a simple to solve, and globally needed tweak?

Straight reroll of #8.

Here's an updated patch that honors #4/#9.

In combination with a tablesort header, this will only work once D8 Core bug #2014745: Exposed sorts break custom style plugin sorting is fixed. Still, the tablesort header doesn't honor our sort handler setting, neither does it honor the field handler's 'display_name' setting. The latter is what it probably should do on default.

Here's the actual patch.

This patch mostly looks great, thanks!

I mostly found minor nits. A few points of substance.

1. +++ b/src/Plugin/views/sort/CountryCode.php
   @@ -0,0 +1,115 @@
   + * Basic sort handler for country's.
   

   s/country's/countries/

   Also, why is this "Basic"? ;) Maybe just: "Sort handler for countries." ?

2. +++ b/src/Plugin/views/sort/CountryCode.php
   @@ -0,0 +1,115 @@
   + * This handler allows to sort by country name. As sorting country codes
   + * alphabetically does not sort the same as country name.
   

   These are awkward. How about:

   "This handler allows sorting by country names, since sorting country codes alphabetically does not produce the same results."

   ?

3. +++ b/src/Plugin/views/sort/CountryCode.php
   @@ -0,0 +1,115 @@
   + * @ViewsSort("country_code")
   

   I wonder if the plugin ID should just be "country" since this is exactly *not* sorting by "country_code". ;)

   Oh, sorry, I see now it's optional to use name vs. code. Still, I wonder...

4. +++ b/src/Plugin/views/sort/CountryCode.php
   @@ -0,0 +1,115 @@
   +class CountryCode extends SortPluginBase {
   

   Ditto the class name? Just "Country"?

5. +++ b/src/Plugin/views/sort/CountryCode.php
   @@ -0,0 +1,115 @@
   +        $when[] = "WHEN $field_name = '$country_code' THEN " . $i++;
   

   Country codes do not and will never include a ' in them, right? Can we be so sure? Should we assert() (run time) about it so we die hard instead of creating invalid SQL? Anything else we can/should do here to be defensive about this? I always get nervous with string literals in SQL generating code...

6. +++ b/src/Plugin/views/sort/CountryCode.php
   @@ -0,0 +1,115 @@
   +      '#description' => $this->t('Sort by country name instead of country code'),
   

   s/code/code./

   I wonder if the UI would be more obvious if this element was called 'Sort by' and there were radios for 'Country name' vs. 'Country code'? No #description needed. ;)

Re 5:
ISO country codes do not and won't ever include anything but two letters, see https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2.
We can still do some doublecheck, but it shouldn't be necessary in this particular case.

Re 6:

I wonder if the UI would be more obvious if this element was called 'Sort by' and there were radios for 'Country name' vs. 'Country code'? No #description needed.

LOL. I had it exactly that way for exactly that reason, but decided to roll back for two reasons:

1. Didn't want to include more changes in the patch than absolutely necessary, to make reviewing easier.
2. Consistency with the setting "Display the country name instead of the country code" in the country_code views field.

But while we should be consistent, I agree that all of them: the field, the filter and the sort should probably be named just "Country" and both the field and the sort should have radios presenting both options equally, rather than offering just a checkbox that kind of overrides the default behavior. Finally, even if internally, displaying and sorting by the country code might be the more straightforward thing to do, in most usecases the actual country name will be the better choice.

So at least I'm going to open a followup to get field and filter in line. This and another patch will be following tonight. This while I'm currently trying to limit the filter's select to the actually existing values (Views Selective Filters style).

Re: #16.5: Yeah, that's what I thought, just being paranoid^H^H^H^H^H^H^H^H careful. ;)

Re: #16.6: LOL. ;) I eagerly await your other patches. Agreed on consistency.

Thanks!
-Derek

Here's another patch:

• implementing our new sort handler for address_country fields as well
• refactoring the option checkbox into radios, the two options being constants, with sort by country name being the default.
• moved the options element a bit up so it comes ahead of ASC/DESC
• added schema
• reworded comments, removed unnecessary description

I didn't rename the whole class in this patch, as otherwise it the interdiff would be useless. The rename comes in the next patch.

Now here's the same with the class being renamed to 'Country'. No other changes.

Nice. And here are some screenshots, showing that everything works as designed now.

The options form:

And these are the actual sort results:

	Interface language: English	Interface language: German
Before / Sort by code:
After / Sort by name:

Love it! I think this is great. 2 very minor nits, then this is RTBC:

1. +++ b/src/Plugin/views/sort/Country.php
   @@ -0,0 +1,132 @@
   + * Sort handler for sorting by either of country code or name.
   

   I'd remove 'of' from this. "... either country code or name."

2. +++ b/src/Plugin/views/sort/Country.php
   @@ -0,0 +1,132 @@
   +      '#weight' => -0.5,
   

   Huh? ;) I've never seen fractional weights like this. Isn't this the only option? Why is this needed?

Edit: dreditor was being weird and duplicated the 1st point. There are only 2 items here.

This fixes both points from #21:

21.1: Gone.

21.2: I see I was wrong, there's also the 'Order by' (duh, you already said that). But, when I removed the fractional #weight, it still gets the ordering right: (first sort by, then order by).

Also added a test for this, using the lovely set of countries and codes from the screenshots in #20. You can keep that dreaded "Needs tests" tag off of here! ;)

Pretty sure this is now RTBC, but I'll let someone else decide that.

Cheers,
-Derek

This looks great, great job dww and Pancho!

I have two minor nitpicks:

+    $this->langcode = $language_manager->getCurrentLanguage()->getId();
+    $this->countryRepository = $country_repository;

The two properties (countryRepository and langcode) are not assigned in the order in which they're defined.

+      foreach (array_keys($country_list) as $country_code) {
+        $when[] = "WHEN $field_name = '$country_code' THEN " . $i++;
+      }

Still a possibility of someone flagging this as a potential SQL injection. Let's avoid that.
We can add an asset(strlen($country_code) == 3) in the foreach, which will guarantee that the code is properly formatted and safe.

Also, I've made dww a co-maintainer, since he's been productive in these queues for a long time.
Derek, feel free to commit this once my review has been addressed.

#21-2 / #22-2:

when I removed the fractional #weight, it still gets the ordering right

The fractional weight might look a bit weird, but it's necessary.

• $form['expose']['label'] has a #weight of -1, see SortPluginBase::buildExposeForm()
• $form['order'] (the ASC/DESC radios) has a default #weight of 0, see: SortPluginBase::showSortForm().

So in order to appear in between the two, our $form['sort_by'] radios unfortunately need a fractional #weight of -0.5.

#23-1:

The two properties (countryRepository and langcode) are not assigned in the order in which they're defined.

Fixed.

#23-2:

The two properties (countryRepository and langcode) are not assigned in the order in which they're defined.

Alright. To be on the safe side, I'm now even doing an
if (assert(preg_match('/^[A-Z]{2}$/', $country_code))){}.
That should be enough, however... :P
[edit: Do we need the assert() though?]

#23-3:

Also, I've made dww a co-maintainer, since he's been productive in these queues for a long time.

Congratulations! :)
Gonna help you as much as I can to get address module polished.

+        if (assert(preg_match('/^[A-Z]{2}$/', $country_code))) {

It doesn't make sense to wrap an assert() in an IF, because an assert() dies if the condition is FALSE.
Maybe we remove the assert and keep the IF, skipping malformed items?
I wanted us to use strlen($country_code) == 3)because I figure it's faster than running preg_match() once per currency (== over 200 calls).

I'm unsure if strlen() makes a measurable difference to a sane regex. preg_match() is fast if the needle matches right at the start.

But here we go, same with strlen($country_code) == 2 and without the redundant assert().

Looks like dww is busy, so I'll wrap this up.

Tweaked a few comments, removed an unused use statement in the test, and committed.

Thanks, everyone!

Indeed, sorry I'm busy, and thanks for wrapping it up!

Cheers,
-Derek

Awesome, thanks! :)

Sorry, this works only for English?

I have Russian 'Administrative Areas' like "Magadan Oblast", "Volgograd Oblast". In Russian correct sorting is:

1) Волгоградская область
2) Магаданская область

But in fact, it sorts in English:

1) Magadan oblast
2) Volgograd Oblast

And displays in Russian this:

1) Магаданская область
2) Волгоградская область

But in Russian this is not correct sorting.