4

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).

5
  • 1
    Calling @Vinai on this one. May 30 '16 at 11:33
  • "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." why don't you have two classes in that case ? May 30 '16 at 11:36
  • 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 workaround would be to just add \Proxy to one of the dependencies :)
    – Vinai
    May 30 '16 at 11:36
  • @RaphaelatDigitalPianism I was trying to use virtual types because the difference is only in configuration. The save paths are different basically.
    – Marius
    May 30 '16 at 11:38
  • 1
    @Vinai. Hmm...proxy might work. It feels dirty, but it should work. You can write your comment as an answer if you can add some code to it :)
    – Marius
    May 30 '16 at 11:39
6

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.

4
  • Nice idea. One question. Can I just simulate in my controller the uploader pool? I mean have something protected $uploaders = []; pass the uploaders using di.xml (I don't care how large this file gets). and have an uploader for images and one for files? Then have a method called getUploader($type) that would check if isset($this->uploaders[$type])? (of course check the if that uploader is an instance of a certain class or interface)
    – Marius
    May 30 '16 at 13:25
  • Better not to do so, as your controller should only be responsible for handling request/response and passing it to models and business logic should be in the model without the direct connection to the way how this data is transferred. May 30 '16 at 13:32
  • Seams like good reasoning. I will try it and get back with the results. Thanks.
    – Marius
    May 30 '16 at 13:38
  • It worked. I used an uploader pool. For now I just add the different uploader types via di.xml to the uploader pool. I don't use an uploader configuration interface. but I might refactor it soon. Thank you
    – Marius
    May 30 '16 at 14:11
4

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.

8
  • 1
    I'll give it a shot and tell you if the tests are smart enough.
    – Marius
    May 30 '16 at 11:43
  • TL;DR. It does not work. Long version. It does not work because I cannot configure the proxy instance via di.xml. I cannot set in my case the allowed extensions or the upload file path because the proxy does not have this properties. I can only configure it via php code calling ->setAllowedExtensions() and I'm trying to avoid this. On the other hand, I can confirm that the static tests are not that smart to detect the proxy of the same class used before.
    – Marius
    May 30 '16 at 12:48
  • Can't you just add the \Proxy in the constructor signature? I mean, it's an ugly workaround in anyway ;)
    – Vinai
    May 30 '16 at 12:50
  • I did, but subject in the proxy is an instance of a "clean" \Sample\News\Model\Uploader. And I cannot configure the subject via di.xml. My uploader class expects as construct parameters a base upload path and the allowed extensions. It is similar to one of the uploader classes in the core (don't remember which one :D). Also there is an other issue. The proxy is not interceptable and I might need it to be.
    – Marius
    May 30 '16 at 12:58
  • But you configure the instance arguments via the virtual type, don't you? Also interception on the base type still works, I think.
    – Vinai
    May 30 '16 at 13:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.