Catalog ProductList helper has coupling to Registry #27119 #27648

mcspronko · 2020-04-08T07:35:50Z

Description (*)

In this pull request the Magento\Framework\Registry class dependency is removed from the Magento\Catalog\Helper\Product\ProductList class.

The new Magento\Catalog\Model\Category\Toolbar\Config class has been added to provide access to the Sort By (Order Field) value.

The getOrderField() method checks if the current category has the default_sort_by value, and returns it, the default configuration value is returned otherwise.

Fixed Issues (if relevant)

Fixes Catalog ProductList helper has coupling to Registry #27119: Catalog ProductList helper has coupling to Registry

Manual testing scenarios (*)

Create a Category and add two Simple Products into the Category
Clear Cache
Verify that the Category opens correctly and uses the default Sort By: Position setting

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)

m2-assistant · 2020-04-08T07:35:52Z

Hi @mcspronko. 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 · 2020-04-09T15:42:42Z

Hi @mcspronko. Thank you for your change, it's really cool. Unfortunately, I have not very good news here. After the internal discussion, the conclusion was the following.
This PR will not be accepted by the product team since there's a code freeze for 2.4 and we cannot make backward-incompatible changes (such as changing signatures of public methods and removing public methods from classes marked as @api).
The changeset for the next MAJOR version for the Catalog module is already defined and is not supposed to be changed.

I'm adding @lenaorobei to the thread for more details.

mcspronko · 2020-04-09T15:49:49Z

@rogyar I can make it backward compatible if this an issue for 2.4. But as far as I am aware, the module-catalog major version is going to be increased. Please confirm

mcspronko · 2020-04-11T12:21:09Z

Dear @rogyar
I've reverted backward-incompatible changes, so you can review the PR.

I appreciate you are looking into it.

rogyar · 2020-04-12T14:33:36Z

Hi @mcspronko. Thanks a lot for the adjustments. Could you take a look at the red static tests, please ("Static tests for B2B" to be precise)? There are some issues that need to be addressed. Some of them are not directly related to your changes but if you have a chance to make it green, it will be awesome. As a rule, the static code analysis issues are pretty minor like missing descriptions or so.

Please, let me know if you need any help from my end.

…egory class

mcspronko · 2020-04-15T05:17:54Z

Hi @rogyar

Please advise what should we do with the failing functional tests, that I believe is somehow random.

Here is the output.

no such element: Unable to locate element: {"method":"css selector","selector":".block.widget.block-compared-products-grid"} (Session info: chrome=65.0.3325.162) (Driver info: chromedriver=2.36.540471 (9c759b81a907e70363c6312294d30b6ccccc2752),platform=Linux 3.10.0-693.17.1.el7.x86_64 x86_64) (WARNING: The server did not provide any stacktrace information) Command duration or timeout: 0 milliseconds For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html Build info: version: '3.8.1', revision: '6e95a6684b', time: '2017-12-01T19:05:32.194Z' System info: host: 'extfat.devops.magento.com', ip: '172.17.0.2', os.name: 'Linux', os.arch: 'amd64', os.version: '3.10.0-693.17.1.el7.x86_64', java.version: '1.8.0_181' Driver info: org.openqa.selenium.chrome.ChromeDriver Capabilities {acceptInsecureCerts: false, acceptSslCerts: false, applicationCacheEnabled: false, browserConnectionEnabled: false, browserName: chrome, chrome: {chromedriverVersion: 2.36.540471 (9c759b81a907e7..., userDataDir: /tmp/.org.chromium.Chromium...}, cssSelectorsEnabled: true, databaseEnabled: false, handlesAlerts: true, hasTouchScreen: false, javascriptEnabled: true, locationContextEnabled: true, mobileEmulationEnabled: false, nativeEvents: true, networkConnectionEnabled: false, pageLoadStrategy: normal, platform: LINUX, platformName: LINUX, rotatable: false, setWindowRect: true, takesHeapSnapshot: true, takesScreenshot: true, unexpectedAlertBehaviour: , unhandledPromptBehavior: , version: 65.0.3325.162, webStorageEnabled: true} Session ID: f8cfb886013fb072a64cf2caec184630 *** Element info: {Using=css selector, value=.block.widget.block-compared-products-grid}

Also, the Semantic Version Checker is red, due to the removal of the optional registry dependency from the ProductList helper class.

rogyar · 2020-04-16T14:22:30Z

@magento run all tests

rogyar · 2020-04-16T16:30:34Z

I see the same functional test failing for other PRs. So, it's not related to the current change somehow.

rogyar · 2020-04-16T16:33:37Z

Since you have didn't introduce a new functionality, there's no necessity for creating functional/integration tests. However, if you could cover the newly introduced classes by unit tests, it would be awesome (you will get extra points for that as well).

Thank you!

…ethod

mcspronko · 2020-04-21T09:01:29Z

@rogyar, please review the latest changes.

rogyar · 2020-04-23T08:36:29Z

app/code/Magento/Catalog/Test/Unit/Helper/Product/ProductListTest.php

+    /**
+     * @var ScopeConfigInterface|MockObject
+     */
+    private $scopeConfig;


Currently, we are advocating suffixing mocks with the Mock keyword. So, it's easier to differentiate mocks and real objects through the test. It's not a requirement, but if you could follow this practice, I would appreciate it.

private $scopeConfigMock; private $toolbarConfigMock;

@rogyar no problem at all. Can you please refer to documentation to read more about the Mock suffix?

Thank you in advance.

Unfortunately, there's no documentation that explicitly describes the best practices for creating unit tests in DevDocs so far. But we are working on the topic on that matter.

sshymko · 2020-07-27T16:14:47Z

app/code/Magento/Catalog/Model/Category/Toolbar/Config.php

+     *
+     * @return string
+     */
+    public function getOrderField(): string


I was thinking how to improve this implementation to be simpler and easier to follow. Here's an equivalent version that avoids duplication of the default behavior.

try { $category = $this->currentCategory->get(); if ($category->getDefaultSortBy()) { return $category->getDefaultSortBy(); } } catch (NoSuchEntityException $exception) { // Fall back to the default configuration } return $this->categoryConfig->getDefaultSortField();

I think, in most cases it's better to keep the "positive" logic inside try..catch as it guarantees that all its preconditions are met (variable $category is defined in this case). Such code is also easier to follow as the sequential logic is grouped together.

mcspronko · 2020-08-07T10:51:34Z

@engcom-Alfa
Any update on this pull request? It's like ages to hear back any feedback and get a positive "approved".

Thank you in advance.

mcspronko · 2020-08-07T10:51:56Z

cc: @rogyar

rogyar · 2020-08-09T14:56:27Z

Hi @mcspronko. Could you merge the latest 2.4-develop changes to your branch and run the tests, please?

Thank you!

ihor-sviziev

Hi @mcspronko,
Could you merge recent changes and run the tests again or give us permissions to do so? Unfortunately we don't have permissions for doing that instead of you.

ihor-sviziev · 2020-10-16T13:26:44Z

@magento run all tests

ihor-sviziev

Hi @mcspronko,
Could you review and fix failing tests?

mcspronko · 2020-10-20T10:30:54Z

@ihor-sviziev I opened tests and I don't see what can be fixed.
The SemVer is not something I decide as part of this PR.

ihor-sviziev · 2020-10-20T10:48:33Z

@magento run all tests

ihor-sviziev · 2020-10-20T10:49:05Z

@mcspronko could you merge recent changes from 2.4-develop branch? Sometimes outdated branch causing test failures

ihor-sviziev · 2020-10-20T12:42:16Z

app/code/Magento/Catalog/Test/Unit/Block/Product/ProductList/ToolbarTest.php

+     */
+    private $toolbarConfig;
+
+    protected function setUp()


Following change broke the tests. Please fix it like this:

Suggested change

protected function setUp()

protected function setUp(): void

ihor-sviziev · 2020-10-20T12:44:10Z

app/code/Magento/Catalog/Test/Unit/Block/Product/ProductList/ToolbarTest.php

@@ -173,11 +192,11 @@ public function testGetPagerEncodedUrl()

        $this->urlBuilder->expects($this->once())
            ->method('getUrl')
-            ->willReturn($url);
+            ->will($this->returnValue($url));


Please revert such changes in all places

ihor-sviziev · 2020-10-20T12:44:38Z

app/code/Magento/Catalog/Test/Unit/Block/Product/ProductList/ToolbarTest.php

        $this->urlBuilder = $this->createPartialMock(Url::class, ['getUrl']);
-        $this->scopeConfig = $this->getMockForAbstractClass(ScopeConfigInterface::class);
+        $this->scopeConfig = $this->createMock(ScopeConfigInterface::class);


Is there any reason for this change?

ihor-sviziev · 2020-10-20T12:46:21Z

app/code/Magento/Catalog/Test/Unit/Block/Product/ProductList/ToolbarTest.php

@@ -7,19 +7,21 @@

 namespace Magento\Catalog\Test\Unit\Block\Product\ProductList;

+use PHPUnit\Framework\TestCase;


Seems like earlier all imports were sorter, while now they're not. Could you run optimize imports in phpstorm for all changes files and commit?

ihor-sviziev · 2020-10-20T12:47:48Z

app/code/Magento/Catalog/Helper/Product/ProductList.php

     */
    public function __construct(
        ScopeConfigInterface $scopeConfig,
-        Registry $coreRegistry = null
+        ToolbarConfig $toolbarConfig


This class marked as @api, so we can't just replace existing argument with a new one. Please introduce new dependecy as optional argument

slavvka · 2020-10-20T16:37:22Z

@magento run all tests

slavvka · 2020-10-20T16:38:19Z

Unit tests failure:

06:03:22 PHP Fatal error:  Declaration of Magento\Catalog\Test\Unit\Block\Product\ProductList\ToolbarTest::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in /var/www/html/app/code/Magento/Catalog/Test/Unit/Block/Product/ProductList/ToolbarTest.php on line 86

ihor-sviziev · 2020-11-09T09:25:31Z

Hi @mcspronko,
Will you be able to update this PR?

sidolov · 2020-11-11T22:32:22Z

@mcspronko I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

m2-assistant · 2020-11-11T22:32:24Z

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

Catalog ProductList helper has coupling to Registry #27119

174bb19

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

magento-engcom-team added Component: Catalog Release Line: 2.4 labels Apr 8, 2020

rogyar self-assigned this Apr 8, 2020

rogyar added the Progress: on hold label Apr 9, 2020

ghost removed the Progress: pending review label Apr 9, 2020

mcspronko requested a review from rogyar April 10, 2020 03:39

Reverted backward incompatible change for the ProductList class

d8dc2c1

mcspronko added 2 commits April 13, 2020 06:10

Fixed static tests, added short method descriptions, moved CurrentCat…

caf2d57

…egory class

Fixed Short description should not be in multi lines

4a7f2b3

rogyar added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: complex Award: bug fix Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests and removed Auto-Tests: Covered All changes in Pull Request is covered by auto-tests labels Apr 16, 2020

rogyar removed the Progress: on hold label Apr 16, 2020

mcspronko added 2 commits April 20, 2020 07:09

Added Unit Tests and fixed the testRemoveOrderFromAvailableOrders() m…

97616ca

…ethod

Fixed static tests

0624642

rogyar reviewed Apr 23, 2020

View reviewed changes

ghost added Progress: review and removed Progress: needs update labels Jun 25, 2020

ghost added Severity: S4 Affects aesthetics, professional look and feel, “quality” or “usability”. Priority: P4 No current plan to fix. Fixing can be deferred as a logical part of more important work. labels Jul 24, 2020

sshymko reviewed Jul 27, 2020

View reviewed changes

ihor-sviziev requested changes Sep 29, 2020

View reviewed changes

ghost assigned ihor-sviziev Sep 29, 2020

ghost added Progress: needs update and removed Progress: review labels Sep 29, 2020

ihor-sviziev added the Progress: review label Oct 16, 2020

ghost removed the Progress: needs update label Oct 16, 2020

ihor-sviziev requested changes Oct 19, 2020

View reviewed changes

ghost added Progress: needs update and removed Progress: review labels Oct 19, 2020

mcspronko requested a review from ihor-sviziev October 20, 2020 10:31

ihor-sviziev requested changes Oct 20, 2020

View reviewed changes

sidolov closed this Nov 11, 2020

m2-community-project bot removed the Progress: needs update label Nov 11, 2020

		@@ -7,19 +7,21 @@

		namespace Magento\Catalog\Test\Unit\Block\Product\ProductList;

		use PHPUnit\Framework\TestCase;

Catalog ProductList helper has coupling to Registry #27119 #27648

Catalog ProductList helper has coupling to Registry #27119 #27648

Uh oh!

Conversation

mcspronko commented Apr 8, 2020 • edited by ihor-sviziev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Apr 8, 2020

Uh oh!

rogyar commented Apr 9, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

rogyar commented Apr 12, 2020

Uh oh!

mcspronko commented Apr 15, 2020

Uh oh!

rogyar commented Apr 16, 2020

Uh oh!

rogyar commented Apr 16, 2020

Uh oh!

rogyar commented Apr 16, 2020

Uh oh!

mcspronko commented Apr 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sshymko Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcspronko commented Aug 7, 2020

Uh oh!

mcspronko commented Aug 7, 2020

Uh oh!

rogyar commented Aug 9, 2020

Uh oh!

ihor-sviziev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihor-sviziev commented Oct 16, 2020

Uh oh!

ihor-sviziev left a comment

Choose a reason for hiding this comment

Uh oh!

mcspronko commented Oct 20, 2020

Uh oh!

ihor-sviziev commented Oct 20, 2020

Uh oh!

ihor-sviziev commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavvka commented Oct 20, 2020

Uh oh!

slavvka commented Oct 20, 2020

Uh oh!

ihor-sviziev commented Nov 9, 2020

mcspronko commented Apr 8, 2020 •

edited by ihor-sviziev

Loading

mcspronko commented Apr 9, 2020 •

edited

Loading

mcspronko commented Apr 11, 2020 •

edited

Loading

sshymko Jul 27, 2020 •

edited

Loading

ihor-sviziev left a comment •

edited

Loading

ihor-sviziev commented Oct 20, 2020 •

edited

Loading