[Closed] FEEDBACK ON BETA of v1.5.5

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.

ibuttons:

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

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?)

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);
}

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

lat9:

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.

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".)

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".

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

includes\templates\template_default\templates\tpl_account_history_info_default.php

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)...

includes\templates\template_default\templates\tpl_checkout_confirmation_default.php

No functional change, but file did change, "not tagged in header". (previously commented out content was simply removed.)

includes\templates\template_default\templates\tpl_shopping_cart_default.php

Modified, but not identified as such...

,

Performing install. Site does not have SSL enabled/available,
Did not click on the "advanced tip" on the first page, but once continued, catalog (storefront) settings included pre-selected Enable SSL for Storefront. Would think that there be some form of internal check first to validate that SSL would be supported before offering the suggestion as default selected.

Also, it seems that the admin server domain being asked up front, if it is different for https: as compared to http: seems like might be better left for a follow on screen?

Somewhat surprised that there are so many "disjointed" fields to fill in their entirety: Domain and URL where URL contains the domain but is not auto updated when the domain is edited.

There is no "back" button, for example when at the database setup, can't return to the system setup section if something were "entered" wrong or needed to be modified. It would appear would have to restart the entire process.

So far during install have also noticed that there is nothing indicating what version this install is to provide... Wouldn't that be something worth having without revealing "too much"?

Database User tip: Is: "... For PCI reasons you should NEVER user 'root' here." could be either: "... For PCI reasons you should NEVER use 'root' here." or "... For PCI reasons you should NEVER use user 'root' here."

Database Host and Database User fields provide auto help if the field is blank and the cursor is placed in the block, but on initial loading the host is populated with localhost and therefore the autohelp is not provided until the field is emptied.

Database Password does not have an autotip (in red below the block) like the previous two, but it does appear when clicking on the field title as described at the top. (Perhaps intermittently applied, recommend review of why and what is to be presented the way(s) it is. Kinda' cool/nice to have the autotip without fear of leaving the page if a link is clicked and without the "popup" information being in the way of starting to type in information.

Database Character Set: Maybe lat9 already made this comment; however, the dropdown selection shows UTF-8, while the help information indicates to possibly use UTF8. Seems like "can't complete the installation" because can't choose what is suggested...

SQL Cache Method tip: Says: "...If your server is really slow, use 'none'. If your site is moderately busy, use 'database'. If your site is extremely high traffic, use 'file'." Also may be something from lat9, but it is not clear if slow is to mean infrequently visited or does not respond quickly.

Auto generated password, would think perhaps it might be possible to have it autogenerate again if "don't like the one provided".

Admin Directory provided, there is a bit of a conflict in description: Says: " We did not change your admin directory automatically as it already seems to have been changed from the default." Thing is, somebody (we?) changed it from the default and it wasn't me... When the files were uploaded an admin directory named admin existed... At this stage of the install, admin does not exist, but the newly created/renamed admin directory does...

Admin Directory tip: Haven't progressed to the next step, but should it indicate that renaming should at least wait until the install is complete?

Finally into the store:

Little surprised that so much of the initial setup (setup wizard) was removed from initial install, but it also somewhat makes sense for those that do the software side. They then let the store owner login and complete the initial information. Also, great that can navigate away from the home page to do other things, though if "needed" to verify home page information and the information was not available for the initial setup, then there does not appear to be a way to "set aside" the initial setup wizard to see how the home page is "coming along". Maybe set the information as a clickable link in the top of the screen (message) after initial display perhaps after initial display per login? (SESSION)

All as one here: related to Developer's Tool kit and hopefully different than what has previously been discussed. Tried to take some obscure actions to just see how things resolved. Overall like the additional options and how the data is highlighted for display.

developer's tool kit: Like the "context" option; however, seems like there could be a little more "clarity" on what it is/what it means, also would be nice if whatever value was entered previously was available again somewhere on screen, either in the results section or back in the box itself (preferred)

Developer's tool kit: search of information in an observer using the look-up classes or things in classes files does not provide results... Example, vanilla install, search in look-up classes or things in classes on: products_viewed_counter extends

No results, place the same search criteria in the Look-up in all files entry and the default observer is searched.

DTK: Not sure what's going on, but I expected different results: using the template search (wanted to see if template override files in the modules directory would be searched since not in the dropdown list for that selection). Entered the following search:

params' => 'class="productListing-hea

Two results returned: both line 69 of: includes/modules/product_listing.php
I expected that the sub-folder responsive_classic would be included...
Search using the "Look-up in all files" option produced the correct result of the same filename in two different folders...
Didn't try searching for information below any change/difference that would change the line number(s).