[Closed] FEEDBACK ON BETA of v1.5.5

[mc12345678]

includes/classes/order.php
generation of a database table id is not collected for potential use before initiating a notify action. lines 888-890.

          zen_db_perform(TABLE_ORDERS_PRODUCTS_ATTRIBUTES, $sql_data_array);

          $this->notify('NOTIFY_ORDER_DURING_CREATE_ADDED_ATTRIBUTE_LINE_ITEM', $sql_data_array);

The $sql_data_array is added to the table Orders Products Attributes, but the position of that addition is not immediately captured and could be lost in the initiation of the notify.

Earlier in the code a similar addition is performed at lines 802-804:

      zen_db_perform(TABLE_ORDERS_PRODUCTS, $sql_data_array);

      $order_products_id = $db->Insert_ID();

      $this->notify('NOTIFY_ORDER_DURING_CREATE_ADDED_PRODUCT_LINE_ITEM', array_merge(array('orders_products_id' => $order_products_id), $sql_data_array));

suggest the same type of designation and assignment:

          zen_db_perform(TABLE_ORDERS_PRODUCTS_ATTRIBUTES, $sql_data_array);

          $order_products_attributes_id = $db->Insert_ID();

          $this->notify('NOTIFY_ORDER_DURING_CREATE_ADDED_ATTRIBUTE_LINE_ITEM', array_merge(array('orders_products_attributes_id' => $order_products_attributes_id), $sql_data_array));

It would seem that though the additional assignment would not be necessary for discovery of the same information in the table (ie. the database table can be searched for successful addition of the $sql_data_array), it is inconsistent with the guidelines and suggestions of the forum for design by NOT collecting the insertion id before performing an action against the $db variable/the database table before collecting that new number and is inconsistent with code a few lines back...

FWIW, this was also posted previously as a code suggestion.

[mc12345678]

Changes made to sql statements in includes/functions/functions_lookups did not use $db->bindVars(

function zen_has_product_attributes_downloads_status($products_id) {
  if (!defined('DOWNLOAD_ENABLED') || DOWNLOAD_ENABLED != 'true') {
    return false;
  }

  $query = "select pad.products_attributes_id
              from " . TABLE_PRODUCTS_ATTRIBUTES . " pa
              inner join " . TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD . " pad
              on pad.products_attributes_id = pa.products_attributes_id
              where pa.products_id = " . (int) $products_id;

  global $db;
  return ($db->Execute($query)->RecordCount() > 0);
}

Could/should be:

function zen_has_product_attributes_downloads_status($products_id) {
  if (!defined('DOWNLOAD_ENABLED') || DOWNLOAD_ENABLED != 'true') {
    return false;
  }

  $query = "select pad.products_attributes_id
              from " . TABLE_PRODUCTS_ATTRIBUTES . " pa
              inner join " . TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD . " pad
              on pad.products_attributes_id = pa.products_attributes_id
              where pa.products_id = :products_id:";

  global $db;
  $query = $db->bindvars($query, ':products_id:', $products_id, 'integer');
  return ($db->Execute($query)->RecordCount() > 0);
}

Also just noticed that this function has been left justified as compared to being indented by two spaces.

[picaflor-azul]

is there any additional documentation on how to install and/or setup the responsive design stuff?

The new template is responsive out of the box.

Thanks,

Anne

[mc12345678]

includes/modules/shopping_cart/header_php.php

includes a closing ?> at the end of the file, even though it has been modified in ZC 1.5.5 (cleanup of files modified to consistently be "touched up")

Also, the header of the header file reflects the last change as in ZC 1.5.4; however, is changed here in ZC 1.5.5.

Similirar issue with checkout_shipping that the header is not modified. (Perhaps the case in several of the includes/modules/pages header_php.php files?)

[lat9]

Changes made to sql statements in includes/functions/functions_lookups did not use $db->bindVars(

function zen_has_product_attributes_downloads_status($products_id) {
  if (!defined('DOWNLOAD_ENABLED') || DOWNLOAD_ENABLED != 'true') {
    return false;
  }

  $query = "select pad.products_attributes_id
              from " . TABLE_PRODUCTS_ATTRIBUTES . " pa
              inner join " . TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD . " pad
              on pad.products_attributes_id = pa.products_attributes_id
              where pa.products_id = " . (int) $products_id;

  global $db;
  return ($db->Execute($query)->RecordCount() > 0);
}

Could/should be:

function zen_has_product_attributes_downloads_status($products_id) {
  if (!defined('DOWNLOAD_ENABLED') || DOWNLOAD_ENABLED != 'true') {
    return false;
  }

  $query = "select pad.products_attributes_id
              from " . TABLE_PRODUCTS_ATTRIBUTES . " pa
              inner join " . TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD . " pad
              on pad.products_attributes_id = pa.products_attributes_id
              where pa.products_id = :products_id:";

  global $db;
  $query = $db->bindvars($query, ':products_id:', $products_id, 'integer');
  return ($db->Execute($query)->RecordCount() > 0);
}

Personally, I think that using bindVars in this case is overkill; the simple recast to (int) as the original code does is sufficient.

[mc12345678]

Personally, I think that using bindVars in this case is overkill; the simple recast to (int) as the original code does is sufficient.

I agree in functionality it is sufficient and in fact the addition of the use of the bindVars invokes additional code execution (in the end bindVars also casts the value to an integer anyways). When is it "right" to use bindVars, when there is more than one field involved? Only when the datatype/source can change on the fly? When the field is non-numeric only thereby using the most secure "conversions" available by the host system?

It's just one of those when making changes go all out and continue to introduce consistency (makes future upgrades/changes much easier). As for style, there are other places where a value is cast as an integer for a sql query and the sql statement has the value captured in single quotes (not the case here), but that is equally "acceptable" but inconsistent with this particular change.

This particular change is interesting in that it approaches things to see first if the database/files even support the code that follows, instead of the previous check of if it was enabled/disabled... A little of a different turn on code compatibility/verification of acceptability for use.

[mc12345678]

admin/attributes_controller.php
header not updated to reflect Changed in ZC 1.5.5.

(FYI, Let me know if I need to stop identifying these because some other "search" or "final edit" is going to be done for such "changes".)

[lat9]

Within the classic_responsive/jscript directory, there are a bunch of minimized javascript files

1. jscript_matchHeight.min.js
2. jquery.mmenu.min.all.js
3. jquery.mmenu.fixedelements.min.js

Having the unminimized versions, too, will be a big help ... just in case anything "goes funky".

[mc12345678]

admin/includes/classes/order.php indicates changed in ZC 1.6.0... Chicken/egg? :)

[mc12345678]

includes\templates\template_default\templates\tpl_account_history_info_default.p hp

header not updated to reflect change in ZC 1.5.5.

within a for loop, which could cause a validation issue:
Line 42 changed from:

      echo '<ul id="orderAttribsList">';

To the more acceptable:

      echo '<ul class="orderAttribsList">';

Haven't looked to see if the CSS changed accordingly or needed to (from #orderAttribsList to .orderAttribsList)...