Skip to content
This repository was archived by the owner on Apr 29, 2019. It is now read-only.

Added $_customerCollection property to Storage class #144

Merged
merged 5 commits into from
Dec 6, 2018
Merged

Added $_customerCollection property to Storage class #144

merged 5 commits into from
Dec 6, 2018

Conversation

maxalmonte14
Copy link
Contributor

@maxalmonte14 maxalmonte14 commented Oct 26, 2018

Description (*)

This PR solves the problem addressed in #135

Fixed Issues (if relevant)

  1. Clean up Model/ResourceModel/Import/Customer/Storage.php #135: Clean up Model/ResourceModel/Import/Customer/Storage.php

Manual testing scenarios (*)

  1. Run ./vendor/bin/phpstan analyse app/code/Magento/CustomerImportExport/Model/ResourceModel/Import/Customer/Storage.php as described in Clean up Model/ResourceModel/Import/Customer/Storage.php #135, after this implementation the following error shouldn't be present
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   Storage.php
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------
  61     Access to an undefined property Magento\CustomerImportExport\Model\ResourceModel\Import\Customer\Storage::$_customerCollection.
 ------ -----------------------------------------------------------------------------------------------------------------------------------------------------------

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)

@dmanners
Copy link
Contributor

Hello, thank you for your pull request. I will start to process this PR and get back to you if I need any more information.

*
* @var \Magento\Customer\Model\ResourceModel\Customer\Collection
*/
private $_customerCollection;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly I cannot see this being used in this class and in fact during the method prepareCollection uses the $this->customerCollectionFactory->create() method to create it's collection. I would suggest that we can remove this variable and the assignment in the __constuct

@maxalmonte14
Copy link
Contributor Author

Done, I removed the unused properties and made sure that StorageTest.php is passing, by running vendor/bin/phpunit app/code/Magento/CustomerImportExport/Test/Unit/Model/ResourceModel/Import/Customer/StorageTest.php 👍

@dmanners
Copy link
Contributor

Nice @maxalmonte14 I will update our tools and progress this.

* @param CollectionByPagesIteratorFactory $colIteratorFactory
* @param array $data
*/
public function __construct(
CustomerCollectionFactory $collectionFactory,
Copy link
Contributor

Choose a reason for hiding this comment

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

So we still need this variable passed in as it get's used on $this->customerCollectionFactory = $collectionFactory; so we should keep that as well as private $customerCollectionFactory; but we can keep the removal of:

        $this->_customerCollection = isset(
            $data['customer_collection']
        ) ? $data['customer_collection'] : $collectionFactory->create();

@maxalmonte14
Copy link
Contributor Author

I added back the customerCollectionFactory property to the class and the parameter to the constructor method, I didn't realize the mistake in the first place, sorry!

dmanners
dmanners previously approved these changes Oct 30, 2018
Copy link
Contributor

@dmanners dmanners left a comment

Choose a reason for hiding this comment

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

Thanks for the speedy update.

@@ -58,9 +58,6 @@ public function __construct(
CollectionByPagesIteratorFactory $colIteratorFactory,
array $data = []
) {
$this->_customerCollection = isset(
Copy link
Contributor

Choose a reason for hiding this comment

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

We are not allowed to remove public properties, because according to PHP Property Visibility: Methods declared without any explicit visibility keyword are defined as public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @maxalmonte14 would you be able to add the $_customerCollection as public then in future versions we can deprecate it. But it would be better at least to have it shown and public now. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, now the $_customerCollection property is public.

@magento-engcom-team magento-engcom-team merged commit bb946c4 into magento-engcom:2.3-develop Dec 6, 2018
@maxalmonte14 maxalmonte14 deleted the improvement/supress_undefined_customerCollection_property_error branch December 6, 2018 18:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants