Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integers being cast to decimal in some particular cases #1198

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

digitalpianism
Copy link
Contributor

Description (*)

This provides a fix for the issue #1192 that I have created.
Details about the fix can be found here: https://magento.stackexchange.com/questions/105244/magento-1-admin-product-grid-column-value-disappear-on-name-search/105421

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Fix integers being cast to decimal in some particular cases #1192

Manual testing scenarios (*)

  1. Run the following script with catalog product flat disabled

require_once 'app/Mage.php';

Mage::app();

$collection = Mage::getModel('catalog/product')->getCollection()
    ->addAttributeToSelect('tax_class_id')
    ->addAttributeToSort('name');

// toggle this to see the weird result
$collection->addAttributeToSelect('price');

// to make the script faster and lighter
$collection->setPageSize(1);

if ($collection->getSize() > 0) {
    var_dump($collection->getFirstItem()->getData('tax_class_id'));
}
  1. Notice the bug.
  2. Apply the fix.
  3. Run the script again.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added the Component: Eav Relates to Mage_Eav label Sep 8, 2020
@tmotyl tmotyl changed the title Fix #1192 Fix integers being cast to decimal in some particular cases Dec 31, 2020
@sreichel
Copy link
Contributor

sreichel commented Aug 30, 2022

This seems not fix the problem ... all tests with flat product tables OFF

<?php

require_once 'app/Mage.php';

Mage::app();

class Test
{
    public function getCollection()
    {
        $collection = Mage::getModel('catalog/product')->getCollection();
        $collection->addAttributeToSelect('tax_class_id');
        $collection->setPageSize(1);
        return $collection;
    }

    public function checkTaxClassId($collection)
    {
        var_dump($collection->getFirstItem()->getData('tax_class_id'));
    }

    public function testPlain()
    {
        $collection = $this->getCollection();
        return $this->checkTaxClassId($collection);
    }

    public function testAddAttributeToSort()
    {
        $collection = $this->getCollection();
        $collection->addAttributeToSort('name');
        return $this->checkTaxClassId($collection);
    }

    public function testAddAttributeToSelect()
    {
        $collection = $this->getCollection();
        $collection->addAttributeToSelect('price');
        return $this->checkTaxClassId($collection);
    }
}

$test = new Test();
$test->testPlain();
$test->testAddAttributeToSort();
$test->testAddAttributeToSelect();

Correct output should be 2

  • output (as it is): string(6) "0.0000"
  • comment addAttributeToSort('name') turns it to string(6) "2.0000"
  • comment addAttributeToSelect('price') returns correctly int(2)

We should also check addAttributeToSort() ...

@digitalpianism
Copy link
Contributor Author

@sreichel I've just tried again on my install and it works fine. Remember I'm running 20.0.2 so maybe something changed along the way if you're using a newer version. This bug is definitely still present in the last official Magento 1 release.

Only one product created, flat tables off.

First run without the fix gives me "2.000", then after applying the fix it gives me "2".

@sreichel sreichel added review needed Problem should be verified needs investigation and removed review needed Problem should be verified labels Aug 30, 2022
@sreichel
Copy link
Contributor

sreichel commented Sep 3, 2022

@digitalpianism tested again with 19.4.x ...

(Can you please check if output is okay with $test->testAddAttributeToSort(); under 20.2?)

  • before fix

    • php7.4
    string(1) "2"
    string(1) "0"
    string(6) "2.0000"
    
    • php8.1
    int(2)
    int(0)
    string(6) "2.0000"
    
  • after fix

    • php7.4
    string(1) "2"
    string(1) "0"
    string(1) "2"
    
    • php8.1
    int(2)
    int(0)
    int(2)
    

@sreichel
Copy link
Contributor

sreichel commented Sep 4, 2022

Tested you example from stackexchange and it works.

... but something is still wrong with this testscript. And we should check why type changes from php7 to 8 ...

@@ -108,7 +108,7 @@ public function getLoadAttributesSelectGroups($selects)
foreach ($selects as $eavType => $selectGroup) {
$mainGroup = array_merge($mainGroup, $selectGroup);
}
return [$mainGroup];
return $mainGroup;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a strong behavior change

@digitalpianism
Copy link
Contributor Author

digitalpianism commented Sep 4, 2022 via email

@sreichel
Copy link
Contributor

sreichel commented Sep 4, 2022

Changed return type seem to come from here ...

https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.pdo.mysql

MySQL Driver
Integers and floats in result sets will now be returned using native PHP types instead of strings when using emulated prepared statements. This matches the behavior of native prepared statements. The previous behaviour can be restored by enabling the PDO::ATTR_STRINGIFY_FETCHES option.

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested.

@sreichel sreichel merged commit c4c2102 into OpenMage:1.9.4.x Sep 19, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit c4c2102. ± Comparison against base commit 028f783.

@luigifab
Copy link
Contributor

luigifab commented Nov 15, 2022

We have a bug with theses changes (OpenMage 20.0.17, PHP 8.0.25 and 8.2.0-rc5, flat catalog enabled for products, also with zf1-future instead of zend - but with zend same problem). I don't understand why.

We have an attribute, text type, id 1276.

On product edit page from OpenMage backend for store 1, I write "abc" and I save, the database is updated (see bellow), but in backend, the default value (123) is displayed (the checkbox use default value is not checked).

image

--

When I revert this PR, if I refresh backend page, the good value (abc) is displayed.

--

Now, if I didn't revert the PR, I can remove the line id 888745 in catalog_product_entity_text, then, when I save "def" from backend, the database is updated, and the good value (def) is displayed.

Now, I update the new line id with the old line id (888745), I refresh backend page, the default value (123) is displayed.

Now, I revert this PR, I refresh backend page, the good value (def) is displayed.

--

With adminer I searched 888745 in all tables of the database, only one result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix integers being cast to decimal in some particular cases
5 participants