[BACKPORT 2.2] [BUGFIX] check if currentStoreId === '' so report isn't generated if … #11915

lewisvoncken · 2017-10-31T22:39:38Z

same as magento-partners/magento2ce#60

Description

When you try to switch to a non-existing or empty store code you the request will result in an Exception.

Fixed Issues (if relevant)

Accessing url with ___store= creates exception #9433: Accessing url with ___store= creates exception

Manual testing scenarios

Try to switch to http://www.example.com/?___store=
This will result in an Exception
Try to switch to http://www.example.com/?___store=nonexistingcode

…url contains ___store= without storecode

dmanners · 2017-11-01T10:02:08Z

There is also a problem with 1 of the integration tests.

There was 1 failure:
1) Magento\Config\Console\Command\ConfigSetCommandTest::testConfigSetValidationError with data set #7 ('general/locale/code', 'en_UK', 'The "wrong_store_code" value ...value.', 'store', 'wrong_store_code')
Symfony\Component\Console\Output\OutputInterface::writeln('<error>Expectation failed for...error>', 0) was not expected to be called more than once.
/home/travis/build/magento/magento2/app/code/Magento/Config/Console/Command/ConfigSetCommand.php:162
/home/travis/build/magento/magento2/vendor/symfony/console/Command/Command.php:266
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Config/Console/Command/ConfigSetCommandTest.php:504
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Config/Console/Command/ConfigSetCommandTest.php:465
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Config/Console/Command/ConfigSetCommandTest.php:304

dmanners

I have a couple of questions but nothing too major here.

dmanners · 2017-11-01T09:53:18Z

app/code/Magento/Store/Model/StoreManager.php


        return $store;
    }

    /**
+     * @return int
+     */
+    public function getCurrentStoreId()


Does this method need to be public? I can only see it being used currently inside this class.

dmanners · 2017-11-01T09:59:08Z

app/code/Magento/Store/Model/StoreManager.php

+        } elseif (key_exists($storeId, $this->storeRepository->getList())) {
+            $store = $this->storeRepository->get($storeId);
+        } else {
+            $store = $this->storeRepository->getById($this->getCurrentStoreId());


I am a bit curious about this else. From what I can see $storeId will be the result of $this->getCurrentStoreId(). Is there any reason you have not just used $storeId here rather than calling $this->getCurrentStoreId()?

Also from what I can see this thing could be done with:

if (is_numeric($storeId) || !key_exists($storeId, $this->storeRepository->getList())) { $store = $this->storeRepository->getById($storeId); } else { $store = $this->storeRepository->get($storeId); }

Or have I missed something out here?

@dmanners $storeId may be passed from outside as a method parameter. And this is actually the issue, as there are several places where ___store query parameter just fetched from a query object and passed to store manager. Store resolver already implements a logic for correct handling of the invalid store code or id.

I'm worrying about this implementation as at this point we do not know exactly that $storeId value comes from ___store query parameter so any code issue will be silently handled without any notification and may lead to unpredictable result and hard to detect bugs.

I think we should find a solution in which only value of ___store query parameter will be affected.

dmanners · 2017-11-10T16:12:32Z

@lewisvoncken have you had any chance to look into this feedback and the tests?

lewisvoncken · 2017-11-11T00:52:19Z

Hi I will try to take a look at it tomorrow

On Fri, 10 Nov 2017 at 17:12, David Manners ***@***.***> wrote: @lewisvoncken <https://github.com/lewisvoncken> have you had any chance to look into this feedback and the tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11915 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFwrF_g-z9p2P1gxr2ABTDuNBvtaU7Hcks5s1HYEgaJpZM4QNhVm> .

-- *Met vriendelijke groet / Best regards,* Lewis Voncken Developer Arthur van Schendelstraat 650 030 - 8200 238 www.experius.nl Volg ons! <https://www.facebook.com/Experius1> <https://www.linkedin.com/company-beta/1008104> <https://twitter.com/experiusnl>

vkublytskyi · 2017-11-13T11:57:01Z

app/code/Magento/Store/Model/StoreManager.php


        return $store;
    }

    /**
+     * @return int
+     */
+    public function getCurrentStoreId()


As @dmanners mentioned there are no reasons to make this method public. For patch releases (2.1-develop and 2.2-develop) we should avoid introduction of new public methods to existing class even if they look as a good addition to a class public interface.

vkublytskyi · 2017-11-13T12:35:47Z

app/code/Magento/Store/Model/StoreManager.php

+        } elseif (key_exists($storeId, $this->storeRepository->getList())) {
+            $store = $this->storeRepository->get($storeId);
+        } else {
+            $store = $this->storeRepository->getById($this->getCurrentStoreId());


@dmanners $storeId may be passed from outside as a method parameter. And this is actually the issue, as there are several places where ___store query parameter just fetched from a query object and passed to store manager. Store resolver already implements a logic for correct handling of the invalid store code or id.

I'm worrying about this implementation as at this point we do not know exactly that $storeId value comes from ___store query parameter so any code issue will be silently handled without any notification and may lead to unpredictable result and hard to detect bugs.

I think we should find a solution in which only value of ___store query parameter will be affected.

lewisvoncken · 2017-11-18T14:59:11Z

@dmanners working on it #magetestfest

dmanners · 2017-11-28T09:21:17Z

HI @lewisvoncken do you need any help with this PR?

dmanners · 2018-01-19T16:29:16Z

Hi @lewisvoncken I am going to close this PR due to inactivity. If you feel like getting back into this code feel free to re-raise this PR at a later date.

sivaschenko · 2018-11-05T17:43:35Z

Hi @lewisvoncken, can you please provide some update on this PR, as you reopened it after a year of inactivity.

sivaschenko · 2018-11-19T09:48:55Z

Closing due to inactivity. Feel free to contact me if you'd like to continue work on this pull request. Thanks for the collaboration!

[BUGFIX] check if currentStoreId === '' so report isn't generated if …

275789d

…url contains ___store= without storecode

dmanners suggested changes Nov 1, 2017

View reviewed changes

dmanners self-assigned this Nov 1, 2017

dmanners added this to the November 2017 milestone Nov 1, 2017

dmanners added Release Line: 2.2 2.2.x bug report Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release labels Nov 1, 2017

magento-team added the Progress: needs update label Nov 1, 2017

magento-engcom-team added the bugfix label Nov 7, 2017

okorshenko removed the Reproduced on develop label Nov 7, 2017

vkublytskyi reviewed Nov 13, 2017

View reviewed changes

okorshenko modified the milestones: November 2017, December 2017 Dec 1, 2017

okorshenko removed the 2.2.x label Dec 14, 2017

okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018

magento-engcom-team added the Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release label Jan 8, 2018

dmanners closed this Jan 19, 2018

lewisvoncken reopened this Oct 30, 2018

sivaschenko self-assigned this Nov 19, 2018

sivaschenko closed this Nov 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BACKPORT 2.2] [BUGFIX] check if currentStoreId === '' so report isn't generated if … #11915

[BACKPORT 2.2] [BUGFIX] check if currentStoreId === '' so report isn't generated if … #11915

Uh oh!

lewisvoncken commented Oct 31, 2017

Uh oh!

dmanners commented Nov 1, 2017

Uh oh!

dmanners left a comment

Uh oh!

dmanners Nov 1, 2017

Uh oh!

dmanners Nov 1, 2017

Uh oh!

dmanners Nov 1, 2017

Uh oh!

vkublytskyi Nov 13, 2017

Uh oh!

dmanners commented Nov 10, 2017

Uh oh!

lewisvoncken commented Nov 11, 2017 via email

Uh oh!

vkublytskyi Nov 13, 2017

Uh oh!

vkublytskyi Nov 13, 2017

Uh oh!

lewisvoncken commented Nov 18, 2017

Uh oh!

dmanners commented Nov 28, 2017

Uh oh!

dmanners commented Jan 19, 2018

Uh oh!

sivaschenko commented Nov 5, 2018

Uh oh!

sivaschenko commented Nov 19, 2018

Uh oh!

Uh oh!

[BACKPORT 2.2] [BUGFIX] check if currentStoreId === '' so report isn't generated if … #11915

[BACKPORT 2.2] [BUGFIX] check if currentStoreId === '' so report isn't generated if … #11915

Uh oh!

Conversation

lewisvoncken commented Oct 31, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Uh oh!

dmanners commented Nov 1, 2017

Uh oh!

dmanners left a comment

Choose a reason for hiding this comment

Uh oh!

dmanners Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

vkublytskyi Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

dmanners commented Nov 10, 2017

Uh oh!

lewisvoncken commented Nov 11, 2017 via email

Uh oh!

vkublytskyi Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

vkublytskyi Nov 13, 2017

Choose a reason for hiding this comment

Uh oh!

lewisvoncken commented Nov 18, 2017

Uh oh!

dmanners commented Nov 28, 2017

Uh oh!

dmanners commented Jan 19, 2018

Uh oh!

sivaschenko commented Nov 5, 2018

Uh oh!

sivaschenko commented Nov 19, 2018

Uh oh!

Uh oh!