v155f: Setting the shipping module/method when no shipping is available

zc1.5.5f. Investigating an issue with One-Page Checkout, I'm trying to ensure that the processing provided by that plugin is as close to the core handling as possible.

Looking at /includes/modules/pages/checkout_shipping/header_php.php, lines 207-211 read:

// If no shipping method has been selected, automatically select the cheapest method.
// If the module's status was changed when none were available, to save on implementing
// a javascript force-selection method, also automatically select the cheapest shipping
// method if more than one module is now enabled
  if ((!isset($_SESSION['shipping']) || (!isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') && zen_count_shipping_modules() >= 1)) $_SESSION['shipping'] = $shipping_modules->cheapest();

... and I'm trying to understand the intent because I think that there might be some issues with that clause, noting the following behavior:

If $_SESSION['shipping'] is not set, then the cheapest is selected without regard to the shipping-module count.
If $_SESSION['shipping']['id'] is not set, then the cheapest is selected without regard to the shipping-module count.

It looks to me like the check wants to prevent calling $shipping_modules->cheapest() if there are no shipping modules available, which begs a further question:

If no shipping modules are available, should $_SESSION['shipping'] be set and, if so, to what?

I've attached a teeny PHP script that illustrates the various combinations and the comparison results.

Attachment #17692

lat9:

... and I'm trying to understand the intent because I think that there might be some issues with that clause, noting the following behavior:

If $_SESSION['shipping'] is not set, then the cheapest is selected without regard to the shipping-module count.
If $_SESSION['shipping']['id'] is not set, then the cheapest is selected without regard to the shipping-module count.

It looks to me like the check wants to prevent calling $shipping_modules->cheapest() if there are no shipping modules available, which begs a further question:

If no shipping modules are available, should $_SESSION['shipping'] be set and, if so, to what?

@lat9, i'm trying to understand the problem. in the 2 examples, you provide above, i agree with the behavior; if not set, select the cheapest.

i disagree that the intent is to prevent calling the cheapest().

if no shipping modules are available, i think that is a bigger problem than this script. how would one complete checkout? it seems the store owner has something mis-configured. and the shipping id would get set to blank.

in looking at your test, the difference is in between clause 4/0 and 4/1. which i think is correct. 4/0 -> we have set shipping, and it is blank, ie no shipping modules are available, so do not do anything. 4/1 -> shipping is set to blank, but there ARE available modules (>=1), so lets set it.

with regards to your tests, which clause combination do you think evaluates incorrectly?

There's a problem in the test.
$a=array('shipping');
As used still evaluates against the first term as not being set and therefore ignoring an in place test of whether a method has been defined by the store as being available. This is though further identified/verified in the cheapest module when executed and returns null from that function instead of a cheapest option.

Historically this logic was implemented to determine if a method was unset by doing unset($_SESSION['shipping']['id']) which would keep the $_SESSION['session'] set, but would evaluate true in the initial logic and possibly true in the number of modules that have been assigned as available.

Thus it is possible that there could be a module assigned to the shop and none are currently active or there could be none assigned at all... both of these conditions are evaluated by cheapest as well as by the function zen_count_shipping_modules(). For the case of none assigned to the store they return null when cheapest is called or 0 if the zen_count_shipping_modules() function is called.

The difference in attempting to call either is that if there truly is no shipping module identified in the store then there would be no session for shipping, but if there had been a shipping module installed, either the shipping session was unset or the shipping module's id was unset and in either case the consideration is that perhaps as a result the cheapest shipping module that exists should be identified as the method to use. If still no module exists or is viable then there should be no shipping method made available and that goes back to one of the design/operation characteristics of ZC that there be at least one shipping module assigned to the store to at least allow checkout. (ie. Free shipping at the least.)

carlwhat:

@lat9, i'm trying to understand the problem. in the 2 examples, you provide above, i agree with the behavior; if not set, select the cheapest.

i disagree that the intent is to prevent calling the cheapest().

if no shipping modules are available, i think that is a bigger problem than this script. how would one complete checkout? it seems the store owner has something mis-configured. and the shipping id would get set to blank.

in looking at your test, the difference is in between clause 4/0 and 4/1. which i think is correct. 4/0 -> we have set shipping, and it is blank, ie no shipping modules are available, so do not do anything. 4/1 -> shipping is set to blank, but there ARE available modules (>=1), so lets set it.

with regards to your tests, which clause combination do you think evaluates incorrectly?
Based on the description of how if $_SESSION['shipping']['id'] is not set that the cheapest option is called without regard to the shipping-module count which would be test 2/0 and 2/1 which both appear to evaluate as always true/being met.

But, the code snippet's method to try to test for $_SESSION['shipping']['id'] not being set doesn't follow the code generation method. Basically, $_SESSION['shipping']['id'] would be unset if first it was set and then unset which provides $_SESSION['shipping'] as an array that has no elements. !isset($_SESSION['shipping']) would evaluate as false, because it is set to an empty array, but !isset($_SESSION['shipping']['id']) would evaluate as true and in a test of 0 modules being assigned then cheapest would not be called, but in a test where there was one module assigned then cheapest would be called.

The code pretty much attempts to identify if cheapest should be called at all, and then if it is likely to need to be called to find other existing ways to further see if cheapest should be called. If a condition exists that either supports the possibility of a cheap method coming back or if it is unknown that there is a method available then to go ahead and attempt to return the cheapest method.

In any of the conditions, the current version of ZC assuming all fles are up-to-date with it doesn't need to test for the number of available modules within this area, because the shipping class cheapest response already handles the associated conditions and will not provide a result if there are no modules available. But... what is the harm in this section remaining as is?

To demonstrate my previous description of the unsetting of the shipping id, I added a fifth "clause" where the variable was first set using the clause 1 criteria but then ['shipping']['id'] was unset. Tested on php 7.1:

Clause1/0 is not satisfied.{"shipping":{"id":"a"}}
Clause2/0 is satisfied.[]
Clause3/0 is satisfied.["shipping"]
Clause4/0 is not satisfied.{"shipping":{"id":""}}
Clause5/0 is not satisfied.{"shipping":[]}

Clause1/1 is not satisfied.{"shipping":{"id":"a"}}
Clause2/1 is satisfied.[]
Clause3/1 is satisfied.["shipping"]
Clause4/1 is satisfied.{"shipping":{"id":""}}
Clause5/1 is satisfied.{"shipping":[]}

carlwhat:

@lat9, i'm trying to understand the problem. in the 2 examples, you provide above, i agree with the behavior; if not set, select the cheapest.

i disagree that the intent is to prevent calling the cheapest().

if no shipping modules are available, i think that is a bigger problem than this script. how would one complete checkout? it seems the store owner has something mis-configured. and the shipping id would get set to blank.

in looking at your test, the difference is in between clause 4/0 and 4/1. which i think is correct. 4/0 -> we have set shipping, and it is blank, ie no shipping modules are available, so do not do anything. 4/1 -> shipping is set to blank, but there ARE available modules (>=1), so lets set it.

with regards to your tests, which clause combination do you think evaluates incorrectly?
@carlwhat, my thought was than the inclusion of the ** && zen_count_shipping_modules() >= 1** clause was meant to prevent the determination of the cheapest ... if there were no shipping modules available.

I agree that the processing is attempting to circumvent additional errors in light of a potential store-misconfiguration.

In my eyes, 2/0 and 3/0 should not cause the cheapest to be selected because there are no shipping modules enabled.

mc12345678:

...<br>
To demonstrate my previous description of the unsetting of the shipping id, I added a fifth "clause" where the variable was first set using the clause 1 criteria but then ['shipping']['id'] was unset. Tested on php 7.1:<br>

Clause1/0 is not satisfied.{"shipping":{"id":"a"}}<br>
Clause2/0 is satisfied.[]<br>
Clause3/0 is satisfied.["shipping"]<br>
Clause4/0 is not satisfied.{"shipping":{"id":""}}<br>
Clause5/0 is not satisfied.{"shipping":[]}<br>
<br>
Clause1/1 is not satisfied.{"shipping":{"id":"a"}}<br>
Clause2/1 is satisfied.[]<br>
Clause3/1 is satisfied.["shipping"]<br>
Clause4/1 is satisfied.{"shipping":{"id":""}}<br>
Clause5/1 is satisfied.{"shipping":[]}<br>

So, in a nutshell, your Clause5 replaces my Clause3 ... which at least lines the results up but still doesn't answer the question as to whether/not the cheapest method should be called *at all* when there are no shipping modules available. That processing, BTW, winds up setting ***$_SESSION['shipping'] = false***;

lat9:

In my eyes, 2/0 and 3/0 should not cause the cheapest to be selected because there are no shipping modules enabled.

i see. and i did agree... but now i do not! as you say cheapest() would set $_SESSION['shipping'] = false;

from:

includes/modules/pages/checkout/header_php.php

  // redirect to calculate shipping on initial load
  if ( !$_SESSION['shipping'] || ( $_SESSION['shipping'] && ($_SESSION['shipping'] == false) && (zen_count_shipping_modules() > 1) ) ) {
    $_SESSION['shipping'] = $shipping_modules->cheapest();
    if (!($messageStack->size('checkout_payment') > 0) && !($messageStack->size('checkout_shipping') > 0) && !($messageStack->size('redemptions') > 0) && $_SESSION['shipping']) {
      zen_redirect(zen_href_link(FILENAME_CHECKOUT, zen_get_all_get_params(), 'SSL'));
    }
  }

shipping needs to be set (even to false) to get past this part of the checkout. else, it will call cheapest cheapest (again), and possibly redirect.

mc,
#1 - i disagree there is a problem with the test. the test illustrated all of the possibilities. as @lat9 said, your example is the same as #3. the slight differentiation only confuses the issue.
#2 - in addition, you state: "...because the shipping class cheapest response already handles the associated conditions and will not provide a result if there are no modules available. But... what is the harm in this section remaining as is?" this is wrong. as @lat9 states, it WILL provide a result, and that result is FALSE. which is a necessary condition to continue the checkout. which is very different from not set. so, there would be significant harm if changed.

best.

carlwhat:

i see. and i did agree... but now i do not! as you say cheapest() would set $_SESSION['shipping'] = false;

from:

includes/modules/pages/checkout/header_php.php

// redirect to calculate shipping on initial load
if ( !$_SESSION['shipping'] || ( $_SESSION['shipping'] && ($_SESSION['shipping'] == false) && (zen_count_shipping_modules() > 1) ) ) {
$_SESSION['shipping'] = $shipping_modules->cheapest();
if (!($messageStack->size('checkout_payment') > 0) && !($messageStack->size('checkout_shipping') > 0) && !($messageStack->size('redemptions') > 0) && $_SESSION['shipping']) {
zen_redirect(zen_href_link(FILENAME_CHECKOUT, zen_get_all_get_params(), 'SSL'));
}
}

> 
> shipping needs to be set (even to false) to get past this part of the checkout.  else, it will call cheapest cheapest (again), and possibly redirect.
> 
> mc,
> #1 - i disagree there is a problem with the test.  the test illustrated all of the possibilities.  as @lat9 said, your example is the same as #3.  the slight differentiation only confuses the issue.
> #2 - in addition, you state: "...because the shipping class cheapest response already handles the associated conditions and will not provide a result if there are no modules available. But... what is the harm in this section remaining as is?"  this is wrong.  as @lat9 states, it WILL provide a result, and that result is FALSE.  which is a necessary condition to continue the checkout.  which is very different from not set.  so, there would be significant harm if changed.
> 
> best.
@carlwhat, you've quoted a section from a modification ... perhaps FEC's ... to the checkout process.  My remembrance is that effort was placed around zc153/154 time to make it so that $_SESSION['shipping'] was either not set or set to an array.  That's where my question comes from.

lat9:

@carlwhat, you've quoted a section from a modification ... perhaps FEC's ... to the checkout process. My remembrance is that effort was placed around zc153/154 time to make it so that $_SESSION['shipping'] was either not set or set to an array. That's where my question comes from.

ok, i am the worst damn record keeper on this board! my apologies to all concerned. that file is NOT part of a base ZC install... it is part of FEC.

based on this "new" information, i see no place where false is used for that session variable. and i agree with you, 2/0 and 3/0 should evaluate to NOT calling the cheapest method.

as a side note, i am a big fan of a single page checkout process. make it easier for the customer to checkout. laborious checkout processes are 1 reason that i think carts get abandoned.

again, sorry for the confusion.

lat9:

So, in a nutshell, your Clause5 replaces my Clause3 ... which at least lines the results up but still doesn't answer the question as to whether/not the cheapest method should be called at all when there are no shipping modules available. That processing, BTW, winds up setting $_SESSION['shipping'] = false;

The processing of the shipping class' cheapest function handles the condition regardless (modules assigned in ZC admin or not and if at least one is then a notifier is also available), the question seems to have become what should be done to see if shipping is in a condition where it would be of benefit to identify a cheapest option (ie. a method is not selected).

Basically if a method hasn't been assigned, then the header_php.php version of the file attempts to choose one. Being unset and one not existing at all are two different things. If I remember correctly, when the test of !isset($_SESSION['shipping']) was added to the list, there was a challenge then about whether the check should include the count of available options or not as well. The result seems to be that the functionality within $shipping->cheapest negated the need for the verification to begin with and was effectively double work, even though there is a nifty function that has a name identifying the type of useful information being provided.

From a line by line everything in the code is human readable, makes sense, is methodical, then I would say that these few lines of code need to be expanded and a little more added; to provide operation, results and perhaps a bit more rigidity, then actually the code could be trimmed down a little.

But in short at this point in the processing (includes/modules/pages/checkout_shipping/header_php.php), the goal is to ensure that a method is chosen (if one exists) or if shipping is not yet customer/store identified then to identify the status of shipping... In looking at just the ZC 1.5.x history of this area of code, there have been several changes to the determination of when to assign $_SESSION['shipping'] to the cheapest value; however, the cheapest function has remained unchanged in all of that... In 1.5.0, the code was similar to what carlwhat has thrown into this mix, basically the only time that the cheapest value was obtained was when $_SESSION['shipping'] was loosely a false value. Yes there was a function shown to maybe "make a difference" but, it wasn't called.

Even in ZC 1.5.0 there was this portion of code in the includes/modules/pages/checkout_shipping/header_php.php file. The second part of the if statement ```
( $_SESSION['shipping'] && ($_SESSION['shipping'] == false) && (zen_count_shipping_modules() > 1) )

never provides a true condition especially that processing never makes it past the second parameter.  Either $_SESSION['shipping'] is somehow false and the remainder of the statement is not processed or it is in some way true which means that it fails the second parameter because if it is true it is not also false.

In the end... what I'm saying is that the header_php.php file for ZC 1.5.5+ would work just fine at determining/assigning a shipping setting if it were:

if ((!isset($_SESSION['shipping']) || !isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') $_SESSION['shipping'] = $shipping_modules->cheapest();

If though the desire is to "feel good" about trying to determine the cheapest module because it is "known" that a module not only exists but is enabled then the code would need to be expanded to:

if (((!isset($_SESSION['shipping']) || !isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') && zen_count_shipping_modules() >= 1)) $_SESSION['shipping'] = $shipping_modules->cheapest();
// This if ensures that $_SESSION['shipping'] has the same value as it would have before the above code change ensuring that there was >=1 shipping module that is both installed and enabled.
if (!isset($_SESSION['shipping'])) {
$_SESSION['shipping'] = false;
]

or when managing/displaying shipping information that it needs to evaluate the absence of a $_SESSION['shipping'] value understanding that if there is no shipping method selected/assigned then when any are displayed, none will be default selected.  This could ultimately be a problem for say PayPal Express checkout where the cheapest method is to be chosen to expedite checkout... 

As far as strict controls go... It looks like for stores that only have in store pickup, they will get a warning on their checkout_shipping page (assuming they even access that page considering only one method exists and would be available) because the cheapest module would skip an instore pickup as an option to choose as cheapest, resulting in no button being selected by default (same "issue" would occur for any module set to be skipped by the cheapest function).  But in a default store this is then caught when accessing the checkout_payment page because $_SESSION['shipping'] is not set sending the code back to the checkout_shipping area.

The question has in a way always been, what defines a method as being not chosen or how should a method be unchosen? Currently it stands within the ZC default shipping modules as $_SESSION['shipping'] being unset, or $_SESSION['shipping']['id'] being unset or == ''... Meaning that it would all work the same if the quantity were checked or not because calling cheapest in any of those conditions would only provide a shipping method if a method was installed, enabled (and not storepickup), otherwise the result (a non-shipment method) would be false. Certainly if it is identified early enough what methods are present then there would be no benefit in calling the cheapest function, why would there be?   

But it doesn't exactly look like there is a need to check for the number of existing/available methods to obtain the result for requesting the cheapest module, it is how does one further process knowing that information.  Even looking at the template code for checkout_shipping, if it determines that there are no installed/enabled shipping methods, then it reports that no methods are available and it doesn't at that point matter what $_SESSION['shipping'] is set to as processing can not continue further.  Now it somewhat does appear to mean that if one knew what the "next" page was that if they have a session that is set to false they could carry on with checkout and not pay for shipping.  So if anything, it seems that cheapest should return null instead of false when no match is found, when shipping is disabled, etc... And/or the header of checkout_payment should inspect $_SESSION['shipping'] differently than it does now... 

So, are you trying to replicate the functionality, or trying to be sure that appropriate options are displayed/made available?

mc12345678:

... So, are you trying to replicate the functionality, or trying to be sure that appropriate options are displayed/made available?
What I'm trying to do is point out a discrepancy in settings for the in-core 3-page checkout ... as I attempt to keep the 1-page checkout "in line". My belief is that the statement

if ((!isset($_SESSION['shipping']) || (!isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') && zen_count_shipping_modules() >= 1)) $_SESSION['shipping'] = $shipping_modules->cheapest();

has misplaced parentheses, causing the cheapest to be returned when there are no shipping modules to be set. Perhaps instead:

if ((!isset($_SESSION['shipping']) || !isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') && zen_count_shipping_modules() >= 1) {
    $_SESSION['shipping'] = $shipping_modules->cheapest();
}

or, using De Morgan's Laws:

if (!(isset($_SESSION['shipping']) && isset($_SESSION['shipping']['id']) && $_SESSION['shipping']['id'] != '') && zen_count_shipping_modules() >= 1) {
    $_SESSION['shipping'] = $shipping_modules->cheapest();
}

lat9:

What I'm trying to do is point out a discrepancy in settings for the in-core 3-page checkout ... as I attempt to keep the 1-page checkout "in line". My belief is that the statement

if ((!isset($_SESSION['shipping']) || (!isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') && zen_count_shipping_modules() >= 1)) $_SESSION['shipping'] = $shipping_modules->cheapest();

> has misplaced parentheses, causing the cheapest to be returned when there are no shipping modules to be set.  Perhaps instead:
> ```
if ((!isset($_SESSION['shipping']) || !isset($_SESSION['shipping']['id']) || $_SESSION['shipping']['id'] == '') && zen_count_shipping_modules() >= 1) {
    $_SESSION['shipping'] = $shipping_modules->cheapest();
}

or, using De Morgan's Laws:

if (!(isset($_SESSION['shipping']) && isset($_SESSION['shipping']['id']) && $_SESSION['shipping']['id'] != '') && zen_count_shipping_modules() >= 1) {
$_SESSION['shipping'] = $shipping_modules->cheapest();
}

That may be, but based on $shipping->cheapest() returning false instead of null that there still runs a potential issue in that $_SESSION['shipping'] can be set but not have a value of "worth" (ie. Can become an issue if used at the end notifier.) and that with the way checkout_payment has been modified that it doesn't reject landing at the checkout_payment page if $_SESSION['shipping'] is set to false... ie instead of checking if !isset($_SESSION['shipping']) it should instead check if empty($_SESSION['shipping'])...

The cheapest() function remains an issue as written in regards to $_SESSION['shipping'] remaining either unset or an array if the only shipping method(s) defined are those that are omitted from being returned as a cheapest option.

Such that even if the final above if statement were used, then $_SESSION['shipping'] would be set to false if the only available (and enabled) option were the instore pickup module...

Whether the function returns *false *or *null *is immaterial ... as the code stands, it will still result in $_SESSION['shipping'] to be set to a non-array value when no shipping is available.

The point is that the clause under-investigation, IMO, should not set an invalid value into $_SESSION['shipping'] if there are no shipping modules available.

lat9:

Whether the function returns *false *or *null *is immaterial ... as the code stands, it will still result in $_SESSION['shipping'] to be set to a non-array value when no shipping is available.

The point is that the clause under-investigation, IMO, should not set an invalid value into $_SESSION['shipping'] if there are no shipping modules available.

To be clear then, what is the definition of a shipping module not being available that is being used?

In the first paragraph state that it will be a non-array value if one is not-available but then in second paragraph describe an invalid value for the same lack of an available method.

What constitutes a valid value? Earlier in the thread (understandably a memory recall/direction possibly subject to more information) indicated that it should either be not set or set to an array. Being set to a non-array is still set and doesn't meet that condition.

mc12345678:

To be clear then, what is the definition of a shipping module not being available that is being used?

In the first paragraph state that it will be a non-array value if one is not-available but then in second paragraph describe an invalid value for the same lack of an available method.

What constitutes a valid value? Earlier in the thread (understandably a memory recall/direction possibly subject to more information) indicated that it should either be not set or set to an array. Being set to a non-array is still set and doesn't meet that condition.
Shipping modules can be configured to be enabled for specific zones only ... or based on other conditions, so it's quite possible for a customer to begin a checkout that can't be fulfilled.

As I said before, I believe that $_SESSION['shipping'] should either not be set or set to an array value (which could be empty).