Refactor moveFileFromTmp to not use a static method call #27581

dmanners · 2020-04-04T14:36:34Z

Description (*)

Currently the class \Magento\Catalog\Model\ImageUploader has a static call to Uploader::getNewFileName

In this PR I plan to:

Create new class for the method getNewFileName,
Refactor the \Magento\Catalog\Model\ImageUploader class to use this new class,

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Upload an image to a category with a unique name and check that name has not been changed,
Upload an image to a category with a nonunique name and check that the name has been updated to be unique,

Questions or comments

I am not too sure on the naming of the new class but would be happy to update it if needed.
I am also not sure about if I need an optional injection of my new class to the ImageUploader class.

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 are green)

Resolved issues:

resolves [Issue] Refactor moveFileFromTmp to not use a static method call #29598: Refactor moveFileFromTmp to not use a static method call

m2-assistant · 2020-04-04T14:36:36Z

Hi @dmanners. 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 give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

rogyar

Hi @dmanners. Thanks a lot for your collaboration. Please, check my comments below regarding some minor adjustments.

lib/internal/Magento/Framework/File/Name.php

dmanners · 2020-04-04T16:37:20Z

Thanks @rogyar I have made the changes required.

I could consider also removing all usages of the old static method as this is used in multiple locations. Though I was thinking about doing this step first then doing the replacments later on.

rogyar · 2020-04-08T07:34:23Z

Hi, @dmanners. Thank you for the adjustments.

If you take a look at the failing static tests, you will see that the ImageUploader class has more than 10 constructor parameters. Basically, there were 10 parameters before your change, but after the file change we have this particular test red and most likely it will be a roadblock for merging the PR.

If you have some time, I would suggest some refactoring of the ImageUploader class. The main idea is to decouple some functional parts into separate class/classes. It does not necessarily mean that in that way we will increase the constructor parameters count. We may just split this class into 2 or more and reduce number of direct dependencies in that way.

Please, let me know if you need any help with that.

Thank you!

dmanners · 2020-04-08T07:57:12Z

Hi @rogyar honestly I cannot see the right place for improvement here without simply removing the logger that is only used for one exception but not the other exception.

If you had any ideas that would be greatly appreciated.

rogyar · 2020-04-12T15:03:06Z

Hi @dmanners. We could create a separate class like ImageUploaderConfigProvider and move all logic and properties related to baseTmpPath, basePath, allowedExtensions and allowedMimeTypes there. Then inject this config provider into the constructor. But we need to take care of all existing virtual types for the image uploader.

dmanners · 2020-04-13T11:47:57Z

app/code/Magento/Catalog/etc/di.xml

@@ -216,7 +216,7 @@
            <argument name="instanceName" xsi:type="string">Magento\Catalog\Model\System\Config\Source\Inputtype</argument>
        </arguments>
    </virtualType>
-    <virtualType name="Magento\Catalog\CategoryImageUpload" type="Magento\Catalog\Model\ImageUploader">


I could only find this one usage of the virtual type though I thought there would be more.

dmanners · 2020-04-13T11:50:16Z

dev/tests/integration/testsuite/Magento/Catalog/Model/ImageUploaderTest.php

-        /** @var $uploader \Magento\MediaStorage\Model\File\Uploader */
-        $this->imageUploader = $this->objectManager->create(
-            \Magento\Catalog\Model\ImageUploader::class,
+        $this->configProvider = $this->objectManager->create(


Not too sure if this is the desired way to do this or if I can inject the virtual type from the di?

dmanners · 2020-04-13T12:35:35Z

app/code/Magento/Catalog/Model/ImageUploader.php

-     *
-     * @return string[]
-     */
-    public function getAllowedExtensions()


Is 2.4 a breaking change? Am I allowed to remove these? I could not see any usaged in place for them apart from in the class.

dmanners · 2020-04-14T07:57:39Z

Hi @rogyar since we have now added in all the actual classes that this is dependant on we are getting the following:

PHP Code Mess has found error(s):
/var/www/html/app/code/Magento/Catalog/Model/ImageUploader.php:14	The class ImageUploader has a coupling between objects value of 13. Consider to reduce the number of dependencies under 13.

Failed asserting that 2 matches expected 0.

What I could suggest is to extract the usage of the store manager and the building of the media url into a ImageUploadURLBuilder class. Though that will only fix this mess warning if I can then remove the storemanager from the constructor. Is that ok with the current release setup or would that break too much compatibility issues?

rogyar · 2020-04-21T08:26:43Z

Hi @dmanners. That's one of the trickiest things in the legacy code refactoring. You try to add some minor adjustment but then you realize that you need to rebuild a huge part of the logic. I believe you know how it works :)

I feel really sorry that you had to spend so much time on that but I don't think that we need to spend more tilting at windmills. Let's do the following. I would ask you to revert the last iteration of refactoring and get back to the place where we had 10 constructor arguments. We will try to convince the gatekeepers that it's the case when we should bypass the test.

Once again, sorry for your time. I wish I could predict such cases and sometimes I can but the legacy code is still stronger than me :(

dmanners · 2020-05-05T12:21:43Z

Hi @rogyar sorry for the slow update. I actually had some changes that I had not pushed before your original message on reverting some commits. Could you please take a look over the current code and let me know which stuff I would need to put back :D

rogyar · 2020-05-11T07:12:05Z

Hi @dmanners. As far as I see, we need all commits prior to b2e726f57326d0dc6e7a5418996b16e9db8fabc7 (including this one).

Thank you

…n preperation for refactoring

…same name are in the directory

engcom-Charlie · 2020-09-04T14:26:00Z

@magento run all tests

rogyar · 2020-09-06T14:16:42Z

Hi @engcom-Charlie @engcom-Delta. I'm not sure what is the reason, but I don't see the particular failing Static/Unit tests in the system. Only a pack of skipped tests.

engcom-Charlie · 2020-09-07T06:25:35Z

@magento run all tests

engcom-Charlie · 2020-09-22T09:32:02Z

Hi, @sidolov can you please help to understand why the unit tests and static tests are falling? In the report, there are no errors.
Thank you.

sidolov · 2020-09-22T19:09:14Z

@engcom-Charlie

PHP Fatal error:  Declaration of Magento\Framework\Test\Unit\File\NameTest::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in /var/www/html/lib/internal/Magento/Framework/Test/Unit/File/NameTest.php on line 33

engcom-Charlie · 2020-09-23T07:37:24Z

@magento run all tests

engcom-Charlie · 2020-09-23T08:19:22Z

@magento run all tests

magento-engcom-team · 2020-09-27T19:57:45Z

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

engcom-Delta · 2020-09-28T13:05:24Z

✔️ QA passed
Result is same as here

engcom-Delta · 2020-09-28T13:06:01Z

Note: Automation tests are passed

…27581

m2-assistant · 2020-09-30T19:39:05Z

Hi @dmanners, 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.

ghost added the Progress: pending review label Apr 4, 2020

magento-engcom-team added Component: Catalog Component: File Component: Test Release Line: 2.4 Event: Global-Contribution-Day labels Apr 4, 2020

rogyar self-requested a review April 4, 2020 15:59

rogyar self-assigned this Apr 4, 2020

rogyar requested changes Apr 4, 2020

View reviewed changes

lib/internal/Magento/Framework/File/Name.php Outdated Show resolved Hide resolved

lib/internal/Magento/Framework/File/Name.php Show resolved Hide resolved

ghost added Progress: needs update and removed Progress: pending review labels Apr 4, 2020

rogyar added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Progress: pending review and removed Progress: needs update labels Apr 4, 2020

dmanners force-pushed the refactor-moveFileFromTmp branch from c81b343 to 4072bc8 Compare April 13, 2020 11:14

dmanners commented Apr 13, 2020

View reviewed changes

dmanners added 3 commits May 19, 2020 08:18

Create a new class for getting new names of files plus a test class i…

c3d14d1

…n preperation for refactoring

Update the test cases to cover the case when multiple files with the …

f2f88a4

…same name are in the directory

Update the Catalog ImageUploader to use our new nameing class

25a9bdf

Merge branch '2.4-develop' into refactor-moveFileFromTmp

e4d45a8

Merge branch '2.4-develop' into refactor-moveFileFromTmp

05abef2

engcom-Charlie added 2 commits September 23, 2020 10:35

Merge branch '2.4-develop' into refactor-moveFileFromTmp

5c31521

add return type

988f31b

fix static

84b22c1

engcom-Charlie requested a review from rogyar September 23, 2020 12:00

ghost added Progress: pending review and removed Progress: extended testing labels Sep 23, 2020

rogyar added the Award: test coverage label Sep 27, 2020

rogyar approved these changes Sep 27, 2020

View reviewed changes

ghost added Progress: ready for testing and removed Progress: pending review labels Sep 27, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Sep 28, 2020

engcom-Delta added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Sep 28, 2020

ghost added Progress: accept and removed Progress: testing in progress labels Sep 28, 2020

magento-engcom-team pushed a commit that referenced this pull request Sep 30, 2020

ENGCOM-8031: Refactor moveFileFromTmp to not use a static method call #…

5192283

…27581

magento-engcom-team merged commit 810b36a into magento:2.4-develop Sep 30, 2020

Refactor moveFileFromTmp to not use a static method call #27581

Refactor moveFileFromTmp to not use a static method call #27581

Uh oh!

Conversation

dmanners commented Apr 4, 2020 • edited by m2-assistant bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

Uh oh!

m2-assistant bot commented Apr 4, 2020

Uh oh!

rogyar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dmanners commented Apr 4, 2020

Uh oh!

rogyar commented Apr 8, 2020

Uh oh!

dmanners commented Apr 8, 2020

Uh oh!

rogyar commented Apr 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmanners Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

dmanners Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

dmanners Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

dmanners commented Apr 14, 2020

Uh oh!

rogyar commented Apr 21, 2020

Uh oh!

dmanners commented May 5, 2020

Uh oh!

rogyar commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

engcom-Charlie commented Sep 4, 2020

Uh oh!

rogyar commented Sep 6, 2020

Uh oh!

engcom-Charlie commented Sep 7, 2020

Uh oh!

engcom-Charlie commented Sep 22, 2020

Uh oh!

sidolov commented Sep 22, 2020

Uh oh!

engcom-Charlie commented Sep 23, 2020

Uh oh!

engcom-Charlie commented Sep 23, 2020

Uh oh!

magento-engcom-team commented Sep 27, 2020

Uh oh!

engcom-Delta commented Sep 28, 2020

Uh oh!

engcom-Delta commented Sep 28, 2020

Uh oh!

m2-assistant bot commented Sep 30, 2020

Uh oh!

Uh oh!

dmanners commented Apr 4, 2020 •

edited by m2-assistant bot

Loading

rogyar commented Apr 12, 2020 •

edited

Loading

rogyar commented May 11, 2020 •

edited

Loading