Added $connection property to class

[maxalmonte14]

Description (*)

This PR solves the problem addressed in #137

Fixed Issues (if relevant)

1. Clean Up Helper/Uploader.php #137: Clean Up Helper/Uploader.php

Manual testing scenarios (*)

Run ./vendor/bin/phpstan analyse app/code/Magento/DownloadableImportExport/Helper/Uploader.php as described in #137, after this implementation the following errors shouldn't be present

------ --------------------------------------------------------------------------------------------------------------------------------
  Line   Uploader.php
 ------ --------------------------------------------------------------------------------------------------------------------------------
  63     Access to an undefined property Magento\DownloadableImportExport\Helper\Uploader::$connection.
 ------ --------------------------------------------------------------------------------------------------------------------------------

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]

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.

[dmanners]

Actually looking into this file I cannot see this being used in this class. So what we can do here is remove this variable and the assignment in the constructor. You will have to leave the varaible in the consturctor definition but can add Add @SuppressWarnings(PHPMD.UnusedFormalParameter) as it will be an unused parameters. For example:

    /**
     * Construct
     *
     * @param \Magento\Framework\App\Helper\Context $context
     * @param \Magento\Downloadable\Helper\File $fileHelper
     * @param \Magento\CatalogImportExport\Model\Import\UploaderFactory $uploaderFactory
     * @param \Magento\Framework\App\ResourceConnection $resource
     * @param \Magento\Framework\Filesystem $filesystem
     *
     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
     */
    public function __construct(
        \Magento\Framework\App\Helper\Context $context,
        \Magento\Downloadable\Helper\File $fileHelper,
        \Magento\CatalogImportExport\Model\Import\UploaderFactory $uploaderFactory,
        \Magento\Framework\App\ResourceConnection $resource,
        \Magento\Framework\Filesystem $filesystem
    ) {
        parent::__construct($context);
        $this->fileHelper = $fileHelper;
        $this->fileUploader = $uploaderFactory->create();
        $this->fileUploader->init();
        $this->fileUploader->setAllowedExtensions($this->getAllowedExtensions());
        $this->fileUploader->removeValidateCallback('catalog_product_image');
        $this->mediaDirectory = $filesystem->getDirectoryWrite(DirectoryList::ROOT);
    }

We do this to keep from breaking backwards compatibility. More information on this topic can be found on devdocs https://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/

[maxalmonte14]

Done. Interesting approach by the way!

[sidolov]

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.

[dmanners]

Ah forgot about that. @maxalmonte14 would you be able to add the $connection 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

[maxalmonte14]

Should I add the property as public with a default null value, or should I leave the implementation as before?

[dmanners]

I would add the property without a default and then put this implementation back as it was before. Unless @sidolov thinks best otherwise.

[maxalmonte14]

Done, now the $connection property is public.

[dmanners]

I guess now we do not need this suppress warning since the connection is being set.

[maxalmonte14]

Done.