Skip to content
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.

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

Fix not detecting current store using store code in url using $storeResolver->getCurrentStoreId() #9429

Merged

Conversation

mimarcel
Copy link
Contributor

@mimarcel mimarcel commented Apr 27, 2017

Description

Current store id is not correctly identified when $storeResolver->getCurrentStoreId() is used.
$storeManager->getStore()->getId() should be used instead.

Fixed Issues

  1. The country drop-down list display incorrect after upgrade to 2.1.4 in Admin #8732: The country drop-down list display incorrect after upgrade to 2.1.4 in Admin

Manual testing scenarios

PRE-REQUISTIES:

  1. Set STORES -> Configuration -> GENERAL -> Web -> Url Options -> Add Store Codes to Urls to Yes.
  2. The system has at least two websites.
  • The first website is set as default website
  • The first website has a store view with code default
  • The second website has a store view with code storeview2.
    stores___settings___stores___magento_admin_
  1. Set STORES -> Configuration -> GENERAL -> General -> Country Options -> Allow Countries to Estonia for first website.
  2. Set STORES -> Configuration -> GENERAL -> General -> Country Options -> Allow Countries to Romania for second website.

STESP TO REPLICATE

  1. Access the website directly using storeview2 store code, e.g. http://local-magento2/storeview2/
  2. Add any product to cart
  3. Go to checkout

EXPECTED

Available country for checkout: Romania

ACTUAL

Available country for checkout: Estonia
Checkout Page

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 on Travis CI are green)

Use $storeManager->getStore()->getId() instead of $storeResolver->getCurrentStoreId()
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Apr 27, 2017

CLA assistant check
All committers have signed the CLA.

@mimarcel mimarcel changed the title 🐛 Fix not detecting current store using store code in url Fix not detecting current store using store code in url Apr 27, 2017
@maghamed maghamed self-requested a review May 3, 2017 15:09
@maghamed maghamed self-assigned this May 3, 2017
@hostep
Copy link
Contributor

hostep commented May 19, 2017

We just were hit by this bug by upgrading a store from Magento 2.1.5 to 2.1.6, but at yet another place.

Namely in the layered navigation. In version 2.1.6 Magento added some extra caching for the layered navigation select attributes, and they also use the super buggy StoreResolver::getCurrentStoreId() method, which doesn't do its job at all, because it always returns the default store id instead of the one on which you are browsing.
See here: https://github.com/magento/magento2/blob/2.1.6/app/code/Magento/Eav/Model/Entity/Attribute/Frontend/AbstractFrontend.php#L255
This was added in this commit: 67b5ff7

Replacing that with $this->storeManager->getStore()->getId() like is done in this PR, resolves the issue.

I think the real solution is to make sure the StoreResolver::getCurrentStoreId() does it job like it is supposed to do, instead of replacing it all over the codebase, but maybe that will cause new issues...

Tnx for the PR @mimarcel, saved me a lot of time and debugging!

@mimarcel
Copy link
Contributor Author

mimarcel commented May 19, 2017

@hostep It's indeed confusing that getCurrentStoreId does not return the final store id. However, the class name is called StoreResolver, therefore it's responsible for resolving the current store id. The StoreManager class is responsible for managing the current store id. The StoreManager uses theStoreResolver, but the current store id can be overwritten by other logic as well by calling setCurrentStore. This is quite tricky since it seems that the issue does replicate in more than one place; it seems it's not really easy to catch even for developers from Magento.

Glad this solution was useful to someone!

@hostep
Copy link
Contributor

hostep commented May 19, 2017

Ok I think this makes sense, thanks for the clarification!
I'll try to create a PR for the Magento 2.1.6 issue we encountered today (maybe tonight, maybe in one of the coming days).

* @param DirectoryHelper $directoryHelper
*/
public function __construct(
\Magento\Directory\Model\ResourceModel\Country\CollectionFactory $countryCollection,
\Magento\Directory\Model\ResourceModel\Region\CollectionFactory $regionCollection,
StoreResolverInterface $storeResolver,
StoreManagerInterface $storeManager,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, due to backwards compatibility policy we may not change constructor arguments. Please add another optional dependency and deprecate the old one if it is not used anymore.

@ishakhsuvarov ishakhsuvarov added this to the June 2017 milestone Jun 21, 2017
@magento-team magento-team merged commit 5475184 into magento:develop Jun 23, 2017
magento-team pushed a commit that referenced this pull request Jun 23, 2017
@mimarcel mimarcel deleted the fix/wrong_allowed_countries_in_checkout branch June 27, 2017 07:47
@mimarcel mimarcel changed the title Fix not detecting current store using store code in url Fix not detecting current store using store code in url using $storeResolver->getCurrentStoreId() Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants