Using 2 instances of the same class as dependencies for an other class

Question

I've built a CRUD module in Magento2.
My Save admin controller has 2 dependencies that are basically the same classes, with different settings.
But when I run the static tests php bin/magento dev:tests:run static I get an error message saying that this is wrong. I should not the same class as a dependency twice for one class.

Data set: array (
  0 => '\\Sample\\News\\Controller\\Adminhtml\\Author\\Save',
)
Argument type duplication in class Sample\News\Controller\Adminhtml\Author\Save in /[ROOT]/app/code/Sample/News/Controller/Adminhtml/Author/Save.php
Multiple type injection [\Sample\News\Model\Uploader]

But I kind of need it like that.
I have to upload 2 things into my entity. An image and a file.
The upload for these 2 behave the same up to only one point, so I have a common class for them, but they are a bit different.
So my controller constructor looks like this:

public function __construct(
    ...
    \Sample\News\Model\Uploader $fileUploader,
    \Sample\News\Model\Uploader $imageUploader,
    ....
)
{
    ....
    $this->fileUploader = $fileUploader;
    $this->imageUploader = $imageUploader;
    ...
}

and in di.xml I have the difference between these 2 classes.

<virtualType name="SampleNewsAuthorImageUploader" type="Sample\News\Model\Uploader"><!-- Virtual type for uploading images -->
    <arguments>
        <argument name="baseTmpPath" xsi:type="const">Sample\News\Model\Uploader::IMAGE_TMP_PATH</argument>
        <argument name="basePath" xsi:type="const">Sample\News\Model\Uploader::IMAGE_PATH</argument>
        <argument name="allowedExtensions" xsi:type="array">
            <item name="jpg" xsi:type="string">jpg</item>
            <item name="jpeg" xsi:type="string">jpeg</item>
            <item name="gif" xsi:type="string">gif</item>
            <item name="png" xsi:type="string">png</item>
        </argument>
    </arguments>
</virtualType>

<virtualType name="SampleNewsAuthorFileUploader" type="Sample\News\Model\Uploader"><!-- virtual type for any file upload -->
    <arguments>
        <argument name="baseTmpPath" xsi:type="const">Sample\News\Model\Uploader::FILE_TMP_PATH</argument>
        <argument name="basePath" xsi:type="const">Sample\News\Model\Uploader::FILE_PATH</argument>
        <argument name="allowedExtensions" xsi:type="array" />
    </arguments>
</virtualType>

<type name="Sample\News\Controller\Adminhtml\Author\Save">
    <arguments><!-- assigne above classes as dependencies for my controller -->
        <argument name="fileUploader" xsi:type="object">SampleNewsAuthorFileUploader</argument>
        <argument name="imageUploader" xsi:type="object">SampleNewsAuthorImageUploader</argument>
    </arguments>
</type>

Is there a way to avoid the duplication of the same dependency or is there an annotation I can use so I won't see the error when I run the tests? Similar to @SuppressWarnings(PHPMD.ExcessiveMethodLength).
(basically I want to hide the dirt under the rug if I cannot do it properly).

Answer 1

It might be good to have same instances as dependencies in some edge cases, but your case is not that edge case scenario. If you have more than one instance of the same kind of dependency, it means you probably need to refactor (use factory or object pool).

I think the best option in your case is to use object pool and NOT to have uploaders passed in directly via di.xml.

Why?

1. As it is visible, you have to configure your uploaders correctly before you pass them into the constructor. If I would instantiate your class directly, it is incredibly easy to make a mistake wich class is responsible for images, and which one for files.

2. You have too much configuration in di.xml itself. It makes sense to introduce a configuration model, that will let you specify allowed file types, per uploader. Imagine adding SVG file type, and it requires you to modify di.xml and re-compile di dependencies, that should not be the case, as it is not business logic change, it is just configuration. Di container is for business logic configuration.

3. When you use DI configuration array argument, it is impossible to remove existing file types.

4. Imagine you will need to add document uploader, that will have a restriction on doc, pdf, rtf? It rings a bell right away that just adding the third dependency is not a good option.

What's a better approach?

1. Create a configuration object that will allow you to specify uploader type per field type that need it.

   interface UploadConfigurationInterface
   {
       public function getTmpDirectory($uploaderType);
       public function getSaveDirectory($uploaderType);
       public function getAllowedFileExtensions($uploaderType);
   }
   

2. Then use it in your uploader pool together with auto-generated uploader factory.

   class UploaderPool
   {
       /** @var UploadConfigurationInterface */
       private $uploaderConfiguration;
       /** @var \Sample\News\Model\UploaderFactory */
       private $uploaderFactory;
       /** @var \Sample\News\Model\Uploader[] */
       private $uploaders;
   
       public function getUploader($uploaderType)
       {
           if (isset($this->uploaders[$uploderType])){
               $this->uploaders[$uploderType] = $this->uploaderFactory->create([
                   'baseTmpPath' => $this->uploaderConfiguration->getTmpDirectory($uploaderType),
                   'basePath' => $this->uploaderConfiguration->getSaveDirectory($uploaderType),
                   'allowedExtensions' => $this->uploaderConfiguration->getAllowedFileExtensions($uploaderType)
               ]);
           }
   
           return $this->uploaders[$uploderType];
       }
   }
   

Even better?

The best one would be to refactor your uploader to use a configuration retrieval per field type to know which types will be uploaded to which directory and which file extension restrictions should be applied. Then you don't need object pool to manage it and your uploader will be independent of configuration logic.

Answer 2

Your use case is perfectly valid, but I don't think you will be able to make that pass the static tests. The static tests are flawed IMO (in more then only this way). The quick and hacky (dirty) workaround would be to just add \Proxy to one of the dependencies :)

public function __construct(
    ...
    \Sample\News\Model\Uploader $fileUploader,
    \Sample\News\Model\Uploader\Proxy $imageUploader,
    ...
)

To be honest, I haven't tested this, but I don't think the static tests are that smart.

Answer 3

I have created UploadPool class

<?php
namespace BA\Rd\Model\Upload;

class UploadPool
{
    protected $uploader;
    const FILE_UPLOADER = 'fileUploader';
    const IMAGE_UPLOADER = 'imageUploader';

    public function __construct(
        array $uploader = []
    ) {
        $this->uploader = $uploader;
    }

    public function getUploader($uploaderType)
    {
        return (object)$this->uploader[$uploaderType];
    }

}

Use the following code to access the directory/tmp path in different class based on the file/image type

protected function getCsvContent($data)
    {
        $uploadDir = $this->uploadPool->getUploader(UploadPool::FILE_UPLOADER); 
        return $this->csv->readCSV($uploadDir->getBaseTmpPath().DIRECTORY_SEPARATOR.$csvfile);
    }

The above code worked for me.