YAMB: Creditmemo with shippingInclTax > 0 and shippingAmount = 0

| Tags: Admin, bug, creditmemo, yamb

I think I just found yet another Magento Bug (YAMB).

I'm working on credit memos and want to add tax to adjustments (I hope I'll write a blog post about this soon - please remind me if you are interested).

I had the problem, that in \Mage_Sales_Model_Order_Creditmemo_Total_Shipping::collect the shippingAmount was 0 (as expected) but the shippingIncludingTax is the complete shipping amount (and is printed on our credit memos).

The problem ends here right before the end of the method.

$creditmemo->setShippingAmount($shipping);
$creditmemo->setBaseShippingAmount($baseShipping);
$creditmemo->setShippingInclTax($shippingInclTax);
$creditmemo->setBaseShippingInclTax($baseShippingInclTax);

I think the bug is the following problem. In the beginning all shipping values are initialized with the values from the order:

$shipping               = $order->getShippingAmount();
$baseShipping           = $order->getBaseShippingAmount();
$shippingInclTax        = $order->getShippingInclTax();
$baseShippingInclTax    = $order->getBaseShippingInclTax();

Due to the fact, that we do it through the backend, the following comment helps us. In the beginning I ignored the comment, and assumed that the baseShippingAmount is 0 and therefore the if shouldn't be entered.

$isShippingInclTax = Mage::getSingleton('tax/config')->displaySalesShippingInclTax($order->getStoreId());

/**
 * Check if shipping amount was specified (from invoice or another source).
 * Using has magic method to allow setting 0 as shipping amount.
 */
if ($creditmemo->hasBaseShippingAmount()) {
    $baseShippingAmount = Mage::app()->getStore()->roundPrice($creditmemo->getBaseShippingAmount());
    if ($isShippingInclTax && $baseShippingInclTax != 0) {
        $part = $baseShippingAmount/$baseShippingInclTax;
        $shippingInclTax    = Mage::app()->getStore()->roundPrice($shippingInclTax*$part);
        $baseShippingInclTax= $baseShippingAmount;
        $baseShippingAmount = Mage::app()->getStore()->roundPrice($baseShipping*$part);
    }

What happens here? We get the current setting for displaySalesShippingInclTax, which should be including tax (but was excluding tax in my case).

So, if the tax setting is "wrong" (excluding tax), we don't enter the if block.

If we enter it, the part of the shipping would be calculated which is refunded (0) and everything is updated. But when the if block is not entered, the shippingInclTax is not updated and therefore the value of shippingInclTax is not consistent with shippingAmount

Images is from Pixabay and CC0
Thanks to 777546-777546

Magento doesn't trigger setup script

| Tags: setup script, Magento, bug

I found a nice new, rarely trigger Magento bug.

In one of our projects, we have an example file to create new products:

data-upgrade-example-new-product-import.php

Beside this we had data scripts:

data-install-1.0.1.php
data-upgrade-1.0.0-1.0.1.php
data-upgrade-1.0.5-1.0.6.php

Unfortunately data-upgrade-1.0.5-1.0.6.php didn't fire.

It took me a while, but I found the problem:

// \Mage_Core_Model_Resource_Setup::_getModifySqlFiles
protected function _getModifySqlFiles($actionType, $fromVersion, $toVersion, $arrFiles)
{
    $arrRes = array();
    switch ($actionType) {

        // ... 

        case self::TYPE_DB_UPGRADE:
        case self::TYPE_DATA_UPGRADE:
            uksort($arrFiles, 'version_compare');
            foreach ($arrFiles as $version => $file) {
                $versionInfo = explode('-', $version);

                // In array must be 2 elements: 0 => version from, 1 => version to
                if (count($versionInfo)!=2) {
                    break;
                }

in $arrFiles we find an array with all files in the data dir which match a certain regex in \Mage_Core_Model_Resource_Setup::_getAvailableDataFiles. In short, when the files starts with data-.

The problem is, that data-upgrade-example-new-product-import.php doesn't meet the if (count($versionInfo)!=2) check and then break is called, which kills the complete loop, but should only be continue;.

So either we hack the core or rename the data-upgrade-example-new-product-import.php, I decided for renaming.

Magento Core Cache Bug

| Tags: core, bug

Thanks very much to my colleagues from iWelt who found this bug and allow me to publish it here to get all the reputation ;-)

Magento can overwrite a cached block with a different one.

Magento Cache Key

Magento generates the cache key by default using the array returned by getCacheKeyInfo:

\Mage_Core_Block_Abstract::getCacheKey
public function getCacheKey()
{
    if ($this->hasData('cache_key')) {
        return $this->getData('cache_key');
    }
    $key = $this->getCacheKeyInfo();
    ...
}

The default implementation of getCacheKeyInfo only takes the name in the layout in consideration:

\Mage_Core_Block_Abstract::getCacheKeyInfo
public function getCacheKeyInfo()
{
    return array(
        $this->getNameInLayout()
    );
}

So far so good.

What is the name of a block, which is created WITHOUT a name?

Block name if no name is given

public function createBlock($type, $name='', array $attributes = array())
{
    ...  
    $block = $this->_getBlockInstance($type, $attributes);
    ...
    $name = 'ANONYMOUS_'.sizeof($this->_blocks);
    ...
    $block->setNameInLayout($name);
    ...
    $this->_blocks[$name] = $block;
    ...
    }
}

As you can see, the name of the block is set automatically. In case you don't know sizeof: it's an alias for count.

So the raising number is the current block count of the whole page.

This means, the name of the block is ANONYMOUS_1, ANONYMOUS_2, etc.

This means, the cache key is generated from [ANONYMOUS_1].

The problem

Now imagine, you have two different pages, which have a block, without a name. These two blocks accidentally have the same sequential number during generation.

Same cache key means: one entry

So in the end you'll get the same cache key and this means: Magento treats it as the same block.

In our case the homepage content block was overwritten by a block of the sidebar.

Solution

My colleague came up with this solution:

We add the current action to the block name:

$name = 'ANONYMOUS_'.sizeof($this->_blocks).Mage::app()->getFrontController()->getRequest()->getPathInfo();

We gave our best to fix the problem itself (which is a wrong generated cache key), but there is no event somewhere in the context of name generation and cache key generation. And because the key is generated in abstract class Mage_Core_Block_Abstract the only solution would be to copy the class to local/Mage which is no solution by definition.

So the rewrite of Mage_Core_Model_Layout is our best bet.