[Solved] BUG in Stock Management when transfer between Stocks (fix included)

[CHitzmann]

I found a Bug in the Advanced Stock Management when you try to transfer not-usable products. You can recreate this, if you do the following steps: * The Stock of WarhouseA is empty (Physical and usable) * Add Products to WarehouseA and set the "usable for sale" to false/no * Now use Transferstock in the Action-List of WarehouseA * set a quantity to transfer * Select usable in source to false/no (there is no usable Stock, so you want to move the physical) * Select WarhouseB as the destination * Finaly you select usable for destination to true/yes * click on transfer -> you will end up with this error: "It is not possible to transfer the specified quantity. No stock was transferred. "

i found the line where i think it goes wrong in classes/stock/StockManager.php in Line 602: public function getPhysicalProductQuantities($productStockCriteria) { ... isset($productStockCriteria['usable']) ? : false ... }

this changes the submitted "usable_from" from false to true because the variable exists (with a value of "false") and therefore isset will return true.

This could be fixed in a simple way: public function getPhysicalProductQuantities($productStockCriteria) { ... isset($productStockCriteria['usable']) ? $productStockCriteria['usable'] : false ... } Now the statement not only checks, if 'usable' exists, it also uses the value of it and defaults to false, if it's missing.

[Traumflug]

Looking at this part of PHP documentation: https://secure.php.net/manual/en/language.operators.comparison.php#language.operators.comparison.ternary, both versions should be actually ~~the same~~ equivalent:

The expression (expr1) ? (expr2) : (expr3) evaluates to expr2 if expr1 evaluates to TRUE, and expr3 if expr1 evaluates to FALSE.

Since PHP 5.3, it is possible to leave out the middle part of the ternary operator. Expression expr1 ?: expr3 returns expr1 if expr1 evaluates to TRUE, and expr3 otherwise.

Are you sure, @CHitzmann?

P.S.: Also, the parameter in question to getProductPhysicalQuantities() is a boolean, so something like this would look fine: php isset($productStockCriteria['usable']) ? true : false

[CHitzmann]

Hello @Traumflug,

yes i'm also wondering, but i can reproduce the error like mentioned above. With the line changed, it works well. But with the original line the error occurs. And you can see on debugging that it changes from false to true. I should say that the Server i'm on using PHP 7.1 May be, that it has changed there again. I will try to test it with a different Version of PHP. But a line with a complete statement makes it independent from what PHP-configuration you're using.... ;)

The line, you mentioned isset($productStockCriteria['usable']) ? true : false also didn't work, because it is the same logic as the original one: isset is returning true in this case, even, if the 'usable' has the value false. So your statement also returns true, but it should be false.

I added the following lines, just to make visible what is happening: Tools::dieOrLog('Test 1: '.Tools::jsonEncode(isset($productStockCriteria['usable'])), false); Tools::dieOrLog('Test 2: '.Tools::jsonEncode(isset($productStockCriteria['usable']) ? true : false), false); Tools::dieOrLog('Test 3: '.Tools::jsonEncode(isset($productStockCriteria['usable']) ? $productStockCriteria['usable'] : false), false); I've added these Lines just above the original return Statement. And this is the result:

The 4th line is because the function is also called for the destination warehouse (where 'usable' is true). 'Test 1...' and 'Test 3... are not logged twice, because logging will skip them, if the same text is logged twice in the same x Seconds.

I will try to redo the test with another Version of PHP. (Yes i could Google, if there has something changed in the behavior of isset between 5.x and 7.1, but this way it is more fun ;) )

Edit: Changed PHP Version to 5.6 and the result is the same.

[CHitzmann]

The problem is not the isset or the syntax of the expression. The Problem is the semantic of it. In the original expression the semantic is: "look, if $productStockCriteria['usable'] exists and return the value of isset, if yes, or false otherwise. But what we really want is: "look, if $productStockCriteria['usable'] exists and return the value of $productStockCriteria['usable'], or false otherwise.

Because in my example the $productStockCriteria['usable'] exist's, so isset will always be true. The original expression will only be false, if the $productStockCriteria['usable'] doesn't exists or if it's value is set to NULL. But the value of $productStockCriteria['usable'] is false. And that is what we really want to know.

"You have the Answer and it is 42. But you don't have the right Question..." ;)

[Traumflug]

In the original expression the semantic is: "look, if $productStockCriteria[‘usable’] exists and return the value of isset, if yes, or false otherwise. But what we really want is: "look, if $productStockCriteria[‘usable’] exists and return the value of $productStockCriteria[‘usable’], or false otherwise.

You nailed it! Now I see it as well. Thanks a ton for your efforts!

Here's the commit: https://github.com/thirtybees/thirtybees/commit/f75ccbf2251b6b875cc3ac0da26c657223212d62

[CHitzmann]

@traumflug said in BUG in Stock Management when transfer between Stocks (fix included):

You nailed it! Now I see it as well. Thanks a ton for your efforts!

Not at all. :blush: