Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions app/code/Magento/Store/Model/StoreManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,24 +152,36 @@ public function isSingleStoreMode()
public function getStore($storeId = null)
{
if (!isset($storeId) || '' === $storeId || $storeId === true) {
if (null === $this->currentStoreId) {
\Magento\Framework\Profiler::start('store.resolve');
$this->currentStoreId = $this->storeResolver->getCurrentStoreId();
\Magento\Framework\Profiler::stop('store.resolve');
}
$storeId = $this->currentStoreId;
$storeId = $this->getCurrentStoreId();
}
if ($storeId instanceof \Magento\Store\Api\Data\StoreInterface) {
return $storeId;
}

$store = is_numeric($storeId)
? $this->storeRepository->getById($storeId)
: $this->storeRepository->get($storeId);
if (is_numeric($storeId)) {
$store = $this->storeRepository->getById($storeId);
} elseif (key_exists($storeId, $this->storeRepository->getList())) {
$store = $this->storeRepository->get($storeId);
} else {
$store = $this->storeRepository->getById($this->getCurrentStoreId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return $store;
}

/**
* @return int
*/
public function getCurrentStoreId()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (null === $this->currentStoreId || '' === $this->currentStoreId) {
\Magento\Framework\Profiler::start('store.resolve');
$this->currentStoreId = $this->storeResolver->getCurrentStoreId();
\Magento\Framework\Profiler::stop('store.resolve');
}
return $this->currentStoreId;
}

/**
* {@inheritdoc}
*/
Expand Down
3 changes: 3 additions & 0 deletions app/code/Magento/Store/Test/Unit/Model/StoreManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ public function testGetStoreStringParameter()
->method('get')
->with($storeId)
->willReturn($storeMock);
$this->storeRepositoryMock->expects($this->atLeastOnce())
->method('getList')
->willReturn([$storeId => $storeMock]);
$actualStore = $this->model->getStore($storeId);
$this->assertInstanceOf(\Magento\Store\Api\Data\StoreInterface::class, $actualStore);
$this->assertEquals($storeMock, $actualStore);
Expand Down