Fix getSize method after clearing data collection

[sergeynezbritskiy]

Description (*)

Fixed the issue with getSize() method in \Magento\Framework\Data\Collection, when we call clear() method after getSize()

Fixed Issues (if relevant)

1. \Magento\Framework\Data\Collection::clear does not clear the result for \Magento\Framework\Data\Collection::getSize #21654: \Magento\Framework\Data\Collection::clear does not clear the result for \Magento\Framework\Data\Collection::getSize

Manual testing scenarios (*)

1. Create any instance of \Magento\Framework\Data\Collection. e.g. \Magento\Sales\Model\ResourceModel\Order\Collection
2. Get count for this collection, call getSize().
3. Change collection select. e.g. add some additional filter by calling \Magento\Framework\Data\Collection\AbstractDb::addFieldToFilter('entity_id', ['lt' => 1]) or \Magento\Framework\Data\Collection::addFilter
4. Call \Magento\Framework\Data\Collection::clear() in order to flush previous result
5. Get count for this collection, call getSize().

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[magento-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @sergeynezbritskiy. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[dmytro-ch]

Hi @sergeynezbritskiy,
thank you for your contribution!

There is an integration test failure with current solution:

1) Magento\Customer\Block\Adminhtml\Edit\Tab\View\CartTest::testGetCollection
Failed asserting that 0 matches expected 1.
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Customer/Block/Adminhtml/Edit/Tab/View/CartTest.php:112

Could you please check this issue?

Also please make sure you sign the Contributor License Agreement before the Magento Community Engineering team can accept your contribution. :)

Thank you!

[sergeynezbritskiy]

Hi @sergeynezbritskiy,
thank you for your contribution!

There is an integration test failure with current solution:

1) Magento\Customer\Block\Adminhtml\Edit\Tab\View\CartTest::testGetCollection
Failed asserting that 0 matches expected 1.
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Customer/Block/Adminhtml/Edit/Tab/View/CartTest.php:112

Could you please check this issue?

Also please make sure you sign the Contributor License Agreement before the Magento Community Engineering team can accept your contribution. :)

Thank you!

Hey @dmytro-ch
I'll check this
Thanks

[sergeynezbritskiy]

Hi @sergeynezbritskiy,
thank you for your contribution!
There is an integration test failure with current solution:

1) Magento\Customer\Block\Adminhtml\Edit\Tab\View\CartTest::testGetCollection
Failed asserting that 0 matches expected 1.
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Customer/Block/Adminhtml/Edit/Tab/View/CartTest.php:112

Could you please check this issue?
Also please make sure you sign the Contributor License Agreement before the Magento Community Engineering team can accept your contribution. :)
Thank you!

Hey @dmytro-ch
I'll check this
Thanks

Unfortunately I had to remove this test, as I found it does not test anything. Moreower the assert is also invalid. This test expects size collection to be 1, but when I check this collection in runtime, it has zero items but Collection::_totalRecords contains 1, So basically this test was based on the issue I fixed with this PR
cc: @dmytro-ch

[dmytro-ch]

@sergeynezbritskiy, thank you for the update!

I briefly checked the implementation for EAV collection.
Seems like the same issue has been already fixed there, by introducing a new private \Magento\Eav\Model\Entity\Collection\AbstractCollection::invalidateSize method and performing the Total Records Count Invalidation in \Magento\Eav\Model\Entity\Collection\AbstractCollection::addAttributeToFilter.

I guess we would be able to apply the same fix for \Magento\Framework\Data\Collection\AbstractDb::addFieldToFilter method.

Looking forward to your thoughts regarding this improvement.

Thank you!

[sergeynezbritskiy]

Thanks @dmytro-ch will review that

[magento-engcom-team]

Hi @dmytro-ch, thank you for the review.
ENGCOM-4527 has been created to process this Pull Request

[orlangur]

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

[sergeynezbritskiy]

@orlangur I checked your suggestions, and fixed issues you mentioned. Please review the PR. Thanks

[sergeynezbritskiy]

a

[orlangur]

@sergeynezbritskiy great! Looks much more readable now and we got rid of useless test 🤝

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-4527 has been created to process this Pull Request

[m2-assistant]

Hi @sergeynezbritskiy, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.