Calling getCurrentUrl on Store will wrongly add "___store" parameter #18941

[Nazar65]

Description (*)

I have the config "store code in URL" set to "yes" but when i use the method "getCurrentUrl" on a Store type variable i get the current URL but with the parameter "___store=[code]" in it, if the current store is not the one requested in the URL.
I want to use the getCurrentUrl method in order to redirect the user to the correct store based on some custom logic (like the browser language), but i don't want any additional parameters to be in the URL.
Form what i found out by looking around in the code, the parameter "___store" is mandatory when the config "use store code in URL" is set to "no" but not when it's set to "yes", as in my case. The store code is already in the URL (as set in system config) and it's not necessary to add any other parameters in the URL to remark this.

Fixed Issues (if relevant)

1. Calling getCurrentUrl on Store will wrongly add "___store" parameter #18941: Calling getCurrentUrl on Store will wrongly add "___store" parameter

Manual testing scenarios (*)

<?php

namespace Fonderia\StoreAutoRedirect\Observer;

use Magento\Framework\Event\Observer;
use Magento\Framework\Event\ObserverInterface;
use Magento\Store\Model\Store;
use Magento\Store\Api\StoreRepositoryInterface;

class RedirectObserver implements ObserverInterface
{
    /**
     * @var StoreRepositoryInterface
     */
    private $storeRepository;

    public function __construct(StoreRepositoryInterface $storeRepository) {
        $this->storeRepository = $storeRepository;
    }

    public function execute(Observer $observer)
    {
        $storeCode = '[your store code]';

        /** @var Store $store */
        $store = $this->storeRepository->get($storeCode);
        $url = $store->getCurrentUrl(true, true);
        // ...
    }
}

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)

[magento-engcom-team]

Hi @Nazar65. 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[Nazar65]

Hi @larsroettig I'm found a better solution for this issue, please look to the newly commit, now the method getCurrentUrl() returns correct value from second store.
before

after

[LucaGallinari]

Any update on this?

[Nazar65]

Hi @larsroettig can you review ?

[larsroettig]

Hi @Nazar65, I will test it in the evening today 👍

[larsroettig]

Change looks good. One thing can you please cover this change with an Integration-Test:
dev/tests/integration/testsuite/Magento/Store/Model/StoreTest.php

[Nazar65]

@larsroettig, yes sure.

[Nazar65]

HI @larsroettig all done.

[larsroettig]

@Nazar65 the static test will fail https://github.com/magento/magento2/pull/19135/files#diff-feec83996906a9185b3d53fb67ab74a1R285 ;-)

Empty Line to mutch :-)

[Nazar65]

@larsroettig oops, fixed :)

[larsroettig]

✅ Code Review
✅ Developer Testing

[sivaschenko]

Hi @Nazar65 , thanks for the contribution! I just would like to clarify the following:

As I can see from the issue description - the expected result is to get an URL that is specific to store but without any "___store" parameters, when requesting a store url in scope of different store having web/url/use_store config setting set to yes (as URL without parameter will look cleaner and nicer).

Is there a way to prevent adding a ___store parameter to url, instead of cutting off the store code after it was added to the url or are there any potential issues you are aware of when omitting the ___store parameter in favour to store as part of url path?

For example, do you think the implementation in https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Store/Url/Plugin/RouteParamsResolver.php#L81 is correct?

[Nazar65]

Hi @sivaschenko I tried to do without it, but it was used in many ways, in the described problem, this method was not adapted for the option "Use store in url", so it was duplicated twice. Then, without this option, if we disable "use store in url" the parameters we will get an address without the title of the bar.

So the problem is simple, we don't need duplicate store code, but the best solution its to leave the functional as it was planned

[Nazar65]

@sivaschenko also this method give you a link like this -> http://localhost/index.php?___store=fixture_second_store&___from_store=default i think this is important

[sivaschenko]

@Nazar65 thank you for the contribution! However, I think we should avoid cutting pieces out of urls generated within the same framework. It would be much better to prevent adding unwanted content to urls in the first place.

I wonder if adding an exclamation mark "!" to the beginning of https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Store/Url/Plugin/RouteParamsResolver.php#L81 condition is something that can help to fix the problem.

Also, it would be great to cover such changes with an integration test without mocking the Url model (as currently it looks more like unit test that is covering only the specific method and not url generation system as a whole)

[sivaschenko]

Considering that URL is generated in Magento framework, not passed from outside, can you please reorganize the code to prevent adding store code to the URL instead of cutting it off after it is added.

[Nazar65]

Method getUrl add parameters to url for specified scope, $url = $this->_url->setScope($this);
Is this right to change this on getUrl method?

[sivaschenko]

@LucaGallinari as an author of the issue, can you please provide your feedback on the fix currently provided. Would removing the store code from URL instead of removing the store URL parameter resolve the issue?

[Nazar65]

@sivaschenko so after add your "!" in "use store in url = 0" we have links like this -> http://magento23dev.vg/?___store=second&___from_store=default
and in "use store in url = 1" have -> http://magento23dev.vg/second/?___from_store=default

also this "!" removed # Method $product->getUrlInStore() returning extremely long URLs, could be a bug
#16310

seems like "!" not related to the issue that was fixed, so we can just add the mark "!", this will solve this issue.

[sivaschenko]

@Nazar65 thanks for the details,

I think that $useStoreInUrl might have been misinterpreted in #16310 As it appears, this config setting is responsible for adding store code to url path, not for adding a parameter.

Considering that store code is added to URL when "use store in url = 1" in Store model I think we shouldn't add it second time (when "use store in url = 1") even if it is requested by _scope_to_url - it is already present in the url. That's the reasoning behind my idea to revert the condition in RouteParamsResolver

In case _scope_to_url is not set - the URL will be short and clean. In case _scope_to_url is set to true - we need to include the store

Also, there are other places around the application where the ___store parameter is added to URL, i.e.:
app/code/Magento/Catalog/Block/Widget/Link.php
app/code/Magento/Store/Block/Switcher.php
app/code/Magento/Store/Model/Store.php
app/code/Magento/Cms/Block/Adminhtml/Page/Grid/Renderer/Action/UrlBuilder.php

Store code is also retrieved from ___store param in the following places:

app/code/Magento/Store/App/Action/Plugin/Context.php
app/code/Magento/Store/Controller/Store/SwitchAction.php

However, store resolver is trying to retrieve store code from URL before getting it from parameter:
\Magento\Store\Model\StoreResolver::getCurrentStoreId

So we need to ensure that removing ___store parameter will not cause any issues with the clients.

[Thundar]

I commented on the original issue, I totally agree with @LucaGallinari and @sivaschenko on restoring the exclamation mark before $useStoreInUrl.
This parameter is used when you have more than one store in the same website. If the user navigates two urls and you mean to switch store on the second url (i.e.: changing language but maintaining the same product page), with the $useStoreInUrl set to 0, the two urls would be the same (eg: https://baseurl.com/catalog/product/view/id/1), and Magento cannot know that it has to change store view without the ___store parameter.
Meanwhile, if $useStoreInUrl is set to 1, the two urls are different, so the ___store parameter is not needed (https://baseurl.com/store_code(1|2)/catalog/product/view/id/1)

From the comments on #16310 , $useStoreInUrl was misunderstood, and not tested when set to yes.

That's why we should revert that line and use !$useStoreInUrl here:
https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Store/Url/Plugin/RouteParamsResolver.php#L81

This should be the only action needed.

[LucaGallinari]

Hi everyone, first of all thank you very much for working on this issue.
I agree with reverting the condition as pointed out by @Thundar and @sivaschenko
And i agree with @sivaschenko on the fact that it's not really clean to remove parameters from the URL after a URL builder added them. It's way better to prevent it. But this is just my point of view.

[Nazar65]

Hi @Thundar thank you for details, @sivaschenko so the best way is to revert mark "!" can we do that ?

[sivaschenko]

@Nazar65 I think yes, feel free to revert $useStoreInUrl value in the condition by adding a ! back

[Nazar65]

@sivaschenko all done :-)

[sivaschenko]

@Nazar65 considering the global effect of the changes it would be great if you can cover them with integration test.

[Nazar65]

@sivaschenko Ok, but i'm already cover this changes with integration test, which scenario i need to cover ?

[sivaschenko]

@Nazar65 sorry, I mean the case when ___store is added to the url.

[Nazar65]

Hi @sivaschenko, i cover changes with integration test, for all places where is used "__store".
in app/code/Magento/Store/Block/Switcher.php and
app/code/Magento/Catalog/Block/Widget/Link.php
this changes not affect.

[magento-engcom-team]

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

[Nazar65]

@sivaschenko I fixed: failed static tests, and unit.

[sivaschenko]

@Nazar65 thank you

[magento-engcom-team]

Hi @Nazar65. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

[hostep]

Hi @sivaschenko!
Can you try to figure out if a backport of this PR and MAGETWO-70726 is coming to the 2.1 release line?
I noticed when upgrading from 2.1.15 to 2.1.16 that switching stores from the homepage no longer works properly, this seems to be caused by #16310
The fix in this PR seems to fix it.
We also need to backport MAGETWO-70726 to have everything store switching related somewhat stable on 2.1.16.

If I understand it correctly, we as a community can no longer send in backports to the 2.1 release line, so I'm wondering if this one is on Magento's list for backporting themselves?

Thanks!