defining product listing default sort order in default_filter.php, faulty iteration

[torvista]

Here I go sticking my neck out again...

Regarding the product listing default sort order code in
/includes/index_filters/default_filter.php…

1) Question

if (isset($_GET['sort']) && strlen($_GET['sort']) > 3) {
  $_GET['sort'] = substr($_GET['sort'], 0, 3);
} 

Why is this limited to three when as far as I can see, the code deals only with two characters eg 2a?

2) Bug (?)

On initial page load $_GET['sort'] is not set.

If there is a sort order defined in the admin, this gets put into $_GET['sort']:

if (!isset($_GET['sort']) and PRODUCT_LISTING_DEFAULT_SORT_ORDER != '') {
    $_GET['sort'] = PRODUCT_LISTING_DEFAULT_SORT_ORDER;
  } 

If
there is no default sort order defined in the admin
OR $_GET['sort'] contains characters not 1-8 or a or d ( Question: Why are column numbers 1-8 allowed when there are only 7 options?)
OR the number character is more than the number of display-enabled columns:
then this loop runs:

for ($i=0, $n=sizeof($column_list); $i<$n; $i++) {
        if (isset($column_list[$i]) && $column_list[$i] == 'PRODUCT_LIST_NAME') {
          $_GET['sort'] = $i+1 . 'a';
          $listing_sql .= " order by p.products_sort_order, pd.products_name";
          break;
        } else {
// sort by products_sort_order when PRODUCT_LISTING_DEFAULT_SORT_ORDER is left blank
// for reverse, descending order use:
//       $listing_sql .= " order by p.products_sort_order desc, pd.products_name";
          $listing_sql .= " order by p.products_sort_order, pd.products_name";
          break;
        }
      } 

As far as I can see this loop is supposed to identify which column is the Product Name and use that for sorting, ascending.
Bug? : However the else clause has a break too, so this loop will only ever execute once and never identify the Product Name column unless it is the first column.

I made this change and it works, i.e. it iterates through all the columns and assigns the correct column id to the $_GET['sort'] for all positions of the Product Name

for ($i=0, $n=sizeof($column_list); $i<$n; $i++) {
if (isset($column_list[$i]) && $column_list[$i] == 'PRODUCT_LIST_NAME') {//get the column with the PRODUCT_LIST_NAME in it and set the $_GET['sort'] to match its position
          //$listing_sql .= " order by p.products_sort_order, pd.products_name";
          break;//break out of the for loop when PRODUCT_LIST_NAME is found
                }
                                }
// sort by products_sort_order when PRODUCT_LISTING_DEFAULT_SORT_ORDER is left blank
// for reverse, descending order use:
//       $listing_sql .= " order by p.products_sort_order desc, pd.products_name";
          $listing_sql .= " order by p.products_sort_order, pd.products_name"; 

3) Question

The next item in this code is:

// if set to nothing use products_sort_order and PRODUCTS_LIST_NAME is off
if (PRODUCT_LISTING_DEFAULT_SORT_ORDER == '') {
        $_GET['sort'] = '20a'; 

I cannot find where the second digit “0” is taken any account of…is this a typo and should be “2a”?

[torvista]

No comment?

[Ajeh]

You will notice in the default code the use of: 20a

that is 3 characters ...

Then you could customize this code in some wild manner and need more than 9 choices which would go to 3 vs 2 characters ...

There are a number of possibilities as to why you might need more than 2 ...

[torvista]

Maybe I'm being dense here as I although I see "20a" is in there, I can only see code that examines the "2" and the "a", not the "0".

and the "bug" with the two breaks, is that a bug?

[Ajeh]

Is this the code that you think is a bug:

// if set to nothing use products_sort_order and PRODUCTS_LIST_NAME is off
if (PRODUCT_LISTING_DEFAULT_SORT_ORDER == '') {
        $_GET['sort'] = '20a';

[torvista]

No, this with two breaks in it. Maybe I didn't explain myself properly above.

for ($i=0, $n=sizeof($column_list); $i<$n; $i++) {
        if (isset($column_list[$i]) && $column_list[$i] == 'PRODUCT_LIST_NAME') {
          $_GET['sort'] = $i+1 . 'a';
          $listing_sql .= " order by p.products_sort_order, pd.products_name";
          break;
        } else {
// sort by products_sort_order when PRODUCT_LISTING_DEFAULT_SORT_ORDER is left blank
// for reverse, descending order use:
//       $listing_sql .= " order by p.products_sort_order desc, pd.products_name";
          $listing_sql .= " order by p.products_sort_order, pd.products_name";
          break;
        }
      } 

[Ajeh]

The way it works is based on how many columns are showing ...

If you have 3 columns, then your choices are:
1a
1d
2a
2d
3a
3d
or Blank, which will be changed in the code to:
20a

I am not understanding your change in the code as in why change that?

I am not able to see what is breaking as changing that one line I am not reproducing any breaks in the code ...

More or less, I am not following your question or able to break anything ...

Can you try typing really, really slow for me with a step by step click for click to break things?

I am not trying to be difficult but I just am not getting the issue or problem ...

[torvista]

What I understand is that this bit is trying to identify if and which of the defined columns is PRODUCT_LIST_NAME by looking at each of the columns that have been enabled, then when it has found PRODUCT_LIST_NAME, assign the column id to the get and then break out of the loop.
If it doesn't find PRODUCT_LIST_NAME, oh well never mind carry on and assign 20a anyway.

What I see is that if PRODUCT_LIST_NAME is not the first defined column, the flow falls through to the ELSE where it breaks out of the loop anyway.
So this code only checks the first column defined, not all of them.

[torvista]

Do I understand correctly what this code is doing?

[gjh42]

I think the "break" statements break out of the if (isset(... sequence, not the for(... loop. Thus $i will continue to be incremented and tests made. At least that would seem to be the case since the stock code does in fact assign sort orders (non-20a) when the product name is not the first element. The demo product setup has the name second, and it has sort order 2a, not 1a as it would be if the name was being tested first (count starts at position 0; 0+1=1).