$product->getUrlInStore() does not allow to override the scope in backend context #21876

Nazar65 · 2019-03-21T14:40:19Z

Description (*)

Magento always uses the 'admin' scope (and the default base url) to retrieve product url and returns incorrect url.

By calling the method $product->getUrlInStore(['_scope' => 2, '_nosid' => true]); I'm always retreiving the product's url with default scope in it. (for example http://default.website/catalog/product/view/...)

Fixed Issues (if relevant)

getProductUrl does not allow to override the scope in backend context #4247 getProductUrl does not allow to override the scope in backend context

Manual testing scenarios (*)

On backend edit page try to get product url from second website

Create 2 websites
Set difference web urls for websites
get product Url on adminhtml in product Edit $product->getUrlInStore(['_scope' => 2, '_nosid' => true]);
You must have url like http://second.website/product.html

Same steps on frontend.

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 · 2019-03-21T14:40:24Z

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

orlangur

Such change is not allowed, see https://devdocs.magento.com/guides/v2.3/contributor-guide/backward-compatible-development/#adding-parameters-in-protected-methods

Nazar65 · 2019-03-21T17:06:42Z

@orlangur thanks, i'm will refactor it.

Nazar65 · 2019-03-22T13:55:40Z

@orlangur wait a little bit i'm still working on this .

Nazar65 · 2019-03-24T19:14:37Z

Hi @orlangur all done ✔️ i'm rechecked and run some test, seems like works correctly from frontend and adminhtml

sivaschenko

Thanks for the pull request @Nazar65 Please see my code review comments

Also, as for original issue - I believe it can be solved by setting current store to the store manager \Magento\Store\Model\StoreManager::setCurrentStore

sivaschenko · 2019-03-27T20:18:10Z

app/code/Magento/Backend/Model/Url.php

+     * @param $scope string
+     * @return \Magento\Store\Model\Store
+     */
+    protected function getScope($scope = null)


Adding protected functions is backward incompatible and we cannot accept that to a patch release.

Also, the proposed implementation is against CQRS principle (getter should not change the state of the system)

@sivaschenko,

Adding protected functions is backward incompatible

What? That's something new to me. Is it documented and then announced somewhere?

Nazar65 · 2019-03-27T21:02:49Z

Hi @sivaschenko thanks for reviewing, but setCurrentStore dosent slove the issue. As when we call url in admin magento always use hardcoded admin scope.

Nazar65 · 2019-03-28T10:33:07Z

@sivaschenko with \Magento\Store\Model\StoreManager::setCurrentStore and my solution

Nazar65 · 2019-03-28T12:37:49Z

@sivaschenko i'm found a better solution, give me some time to test it, thanks 👍

orlangur · 2019-03-28T13:13:53Z

lib/internal/Magento/Framework/Url.php

-        $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());
+        $customScope
+            ? $result = $this->getData('scope')->getBaseUrl($this->_getType(), $this->_isSecure())
+            : $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());


Who taught you this?)

Please use ternary operator normally, $result = $customScope ? ... : ...

Still not fixed.

Nazar65 · 2019-03-28T13:15:49Z

My observation, to get correct product url in adminhtml we need create the url object for the product ->

        $catalogUrl= $this->_objectManager->get(\Magento\Catalog\Model\ResourceModel\Url::class);
        $products = $catalogUrl->getRewriteByProductStore([$product->getId() => 2]);
        $urlData = new \Magento\Framework\DataObject($products[$product->getId()]);
        $prod->setUrlDataObject($urlData);

So after that we have correct url without admin in url. like this -> http://default.website/product1.html
But we not able to get second website url even if we set setCurrentStoreId becouse magento always use hardcoded admin scope.

Nazar65 · 2019-04-02T15:27:30Z

Hi @orlangur Hi @sivaschenko can you review new changes ?

orlangur

@Nazar65 please check my comments. After we finalize the implementation, some tests would really not hurt here.

Please use original ticket #4247 on commit message and make it meaningful.

orlangur · 2019-04-02T15:41:04Z

app/code/Magento/Store/Url/Plugin/RouteParamsResolver.php

@@ -63,6 +63,7 @@ public function beforeSetRouteParams(
    ) {
        if (isset($data['_scope'])) {
            $subject->setScope($data['_scope']);
+            $subject->setRouteParam('_scope',$data['_scope']);


There should be a space here according to PSR-2 but I'm not sure if Travis is not broken currently.

orlangur · 2019-04-02T15:41:33Z

lib/internal/Magento/Framework/Url.php

-        $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());
+        $customScope
+            ? $result = $this->getData('scope')->getBaseUrl($this->_getType(), $this->_isSecure())
+            : $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());


Still not fixed.

orlangur · 2019-04-02T15:43:57Z

lib/internal/Magento/Framework/Url.php

@@ -478,8 +483,8 @@ public function getBaseUrl($params = [])
        ) {
            $this->getRouteParamsResolver()->setType(UrlInterface::URL_TYPE_DIRECT_LINK);
        }
-
-        $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());
+        $result = $customScope ? $this->getData('scope')->getBaseUrl($this->_getType(), $this->_isSecure())


Please explain the logic. Why we cannot always use $this->getData('scope')?

@orlangur Because this metod have usages in many methods and not always sets "_scope" to the dataObjects, and method _getScope() checks if _scope not exist in dataObject then they give us a hardcoded admin "_scope", so basically I added a 1 more scenario where we can get _scope from data object, and this scenario will not break some tests, i think

I think _getScope can be used instead of getData('scope') - it's basically the same plus handling the case when scope is not set.

Nazar65 · 2019-04-05T13:16:19Z

@orlangur I'm tested this a couple day and this works perfect with my custom module, so i'm will write some tests, and will finish on this.

Nazar65 · 2019-04-08T14:47:33Z

HI @orlangur @sivaschenko i'm finished the implementation and cover with intergation tests, seems like works fine.

sivaschenko

Hi @Nazar65 thanks for updates, please see my review notes

app/code/Magento/Store/Url/Plugin/RouteParamsResolver.php

lib/internal/Magento/Framework/Url.php

dev/tests/integration/testsuite/Magento/Catalog/Model/Product/UrlTest.php

sivaschenko · 2019-04-17T16:36:11Z

lib/internal/Magento/Framework/Url.php

@@ -478,8 +483,8 @@ public function getBaseUrl($params = [])
        ) {
            $this->getRouteParamsResolver()->setType(UrlInterface::URL_TYPE_DIRECT_LINK);
        }
-
-        $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());
+        $result = $customScope ? $this->getData('scope')->getBaseUrl($this->_getType(), $this->_isSecure())


I think _getScope can be used instead of getData('scope') - it's basically the same plus handling the case when scope is not set.

Nazar65 · 2019-04-19T08:20:49Z

Hi @sivaschenko thank you for you review notes that's what i needed. now finally bug in this method Framework/Url -> protected function _getScope() in some cases php use parent method in Model/Url protected function _getScope() ignore self method wich have scope in dataObject.

seems like use $tthis->_getScope() or self::_getScope() fails integrations tests, that why i use if statment, and get scope on DataObject $this->getData('_scope');

sivaschenko

Hi @Nazar65 thanks for the update, see my code review comments

sivaschenko · 2019-04-23T12:26:27Z

lib/internal/Magento/Framework/Url.php

+     *
+     * @param array $params
+     */
+    private function setRequestScope($params) :void


Suggested change

private function setRequestScope($params) :void

private function setRequestScope(array $params): void

sivaschenko · 2019-04-23T13:04:14Z

lib/internal/Magento/Framework/Url.php

@@ -478,8 +488,7 @@ public function getBaseUrl($params = [])
        ) {
            $this->getRouteParamsResolver()->setType(UrlInterface::URL_TYPE_DIRECT_LINK);
        }
-
-        $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());
+        $result = self::_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());


I don't think this change from $this->_getScope() to self::_getScope() is needed. Can you please provide more details on why do you think it is necessary

@sivaschenko with self:: we get _getScope() in Framework/Url

2 - with $this we get _getScope() in Model/Url wich returns admin scope.

But this is not good idea to use one method, when we need to call product url in admin scope, i thinks would be better to use previous solution with $this->getData('_scope') and if statement, because some tests will fails with this

I think this solution will be better and not break any functional
$result = $this->setRequestScope($params)->getBaseUrl($this->_getType(), $this->_isSecure());

private function setRequestScope($params) { $customScope = $this->getRouteParamsResolver()->getRouteParam('_scope'); if (isset($params['_scope']) ) { $this->setScope($params['_scope']); } else if ($customScope) { $this->setScope($customScope); return $this->getData('scope'); } return $this->_getScope(); }

sivaschenko

Hi @Nazar65 , please see my propositions the in review comments.

lib/internal/Magento/Framework/Url.php

sivaschenko · 2019-04-25T14:28:38Z

lib/internal/Magento/Framework/Url.php

+    {
+        $customScope = $this->getRouteParamsResolver()->getRouteParam('_scope');
+
+        if (isset($params['_scope'])) {


What do you think about introducing a setScope method to \Magento\Backend\Model\Url to be able to assign custom scope to _scope property? (it's better to also keep parent method call for backward compatibility purposes)

@sivaschenko yes, that's what i needed, now it looks fine, i'm little bit confused by overriding method _getScope()

app/code/Magento/Backend/Model/Url.php

Nazar65 · 2019-04-28T18:42:08Z

@sivaschenko done ✔️

magento-engcom-team · 2019-04-29T10:36:16Z

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

soleksii · 2019-05-03T07:03:14Z

✔️ QA Passed

sivaschenko

Hi @Nazar65 can you please recheck the integration tests, they are falling with the following results

Magento\Catalog\Model\Product\UrlTest::testGetUrlInStoreWithSecondStore
Magento\Framework\Exception\NoSuchEntityException: The product that was requested doesn't exist. Verify the product and try again.

/var/www/html/app/code/Magento/Catalog/Model/ProductRepository.php:276
/var/www/html/dev/tests/integration/tmp/sandbox--3c335e531fab12e41ef1a983a92177df/generated/code/Magento/Catalog/Model/ProductRepository/Interceptor.php:24
/var/www/html/dev/tests/integration/testsuite/Magento/Catalog/Model/Product/UrlTest.php:68

sivaschenko

Hi @Nazar65 thanks for the update!

Please correct the test to utilize the data provider functionality

dev/tests/integration/testsuite/Magento/Catalog/Model/Product/UrlTest.php

Nazar65 · 2019-05-15T10:14:48Z

@sivaschenko done ✔️

magento-engcom-team · 2019-05-30T10:39:44Z

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

m2-assistant · 2019-06-08T22:21:58Z

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

… scope in backend context magento#21876

magento-engcom-team added Component: Backend Component: Url Release Line: 2.3 labels Mar 21, 2019

Nazar65 changed the title ~~getProductUrl does not allow to override the scope in backend context~~ $product->getUrlInStore() does not allow to override the scope in backend context Mar 21, 2019

orlangur requested changes Mar 21, 2019

View reviewed changes

ghost assigned orlangur Mar 21, 2019

ghost added the Progress: needs update label Mar 21, 2019

Nazar65 requested a review from sivaschenko March 26, 2019 17:37

sivaschenko requested changes Mar 27, 2019

View reviewed changes

ghost assigned sivaschenko Mar 27, 2019

orlangur requested changes Mar 28, 2019

View reviewed changes

orlangur requested changes Apr 2, 2019

View reviewed changes

Nazar65 requested a review from sivaschenko April 16, 2019 19:12

sivaschenko requested changes Apr 17, 2019

View reviewed changes

sivaschenko requested changes Apr 23, 2019

View reviewed changes

sivaschenko requested changes Apr 25, 2019

View reviewed changes

sivaschenko requested changes Apr 28, 2019

View reviewed changes

app/code/Magento/Backend/Model/Url.php Show resolved Hide resolved

soleksii self-assigned this May 2, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels May 2, 2019

nmalevanec self-assigned this May 3, 2019

ghost added Progress: extended testing Progress: accept and removed Progress: testing in progress Progress: extended testing labels May 3, 2019

sivaschenko requested changes May 7, 2019

View reviewed changes

ghost added Progress: needs update and removed Progress: accept labels May 7, 2019

sivaschenko requested changes May 10, 2019

View reviewed changes

getUrlInStore() does not allow to override the scope in backend context

c50975a

IvanPletnyov self-assigned this May 20, 2019

ghost added Progress: ready for testing Progress: accept and removed Progress: needs update Progress: ready for testing labels May 20, 2019

Nazar65 requested a review from sivaschenko May 30, 2019 08:34

sivaschenko approved these changes May 30, 2019

View reviewed changes

magento-engcom-team added the Award: bug fix label Jun 7, 2019

magento-engcom-team mentioned this pull request Jun 8, 2019

getProductUrl does not allow to override the scope in backend context #4247

Closed

magento-engcom-team merged commit c50975a into magento:2.3-develop Jun 8, 2019

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request Jun 8, 2019

ENGCOM-4843: $product->getUrlInStore() does not allow to override the…

d1f7b97

… scope in backend context magento#21876

magento-engcom-team added this to the Release: 2.3.3 milestone Jun 8, 2019

	private function setRequestScope($params) :void
	private function setRequestScope(array $params): void

$product->getUrlInStore() does not allow to override the scope in backend context #21876

$product->getUrlInStore() does not allow to override the scope in backend context #21876

Uh oh!

Conversation

Nazar65 commented Mar 21, 2019 • edited by orlangur 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!

magento-engcom-team commented Mar 21, 2019

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Mar 21, 2019

Uh oh!

Nazar65 commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nazar65 commented Mar 24, 2019

Uh oh!

sivaschenko left a comment

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!

Nazar65 commented Mar 27, 2019

Uh oh!

Nazar65 commented Mar 28, 2019

Uh oh!

Nazar65 commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Mar 28, 2019

Uh oh!

Nazar65 commented Apr 2, 2019

Uh oh!

orlangur 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!

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!

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Apr 5, 2019

Uh oh!

Nazar65 commented Apr 8, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nazar65 commented Mar 21, 2019 •

edited by orlangur

Loading

Nazar65 commented Mar 22, 2019 •

edited

Loading

orlangur left a comment •

edited

Loading

Nazar65 commented Apr 19, 2019 •

edited

Loading

Nazar65 Apr 25, 2019 •

edited

Loading