Enable 'back' GET parameter management #13233

tomas862 · 2019-04-08T10:53:00Z

Questions	Answers
Branch?	develop
Description?	adds response event listener which detects if current request stack has "back-url" parameter and redirects to it instead of using original request. If the request url is equal to response url it prevent from such action. E.g "save and stay" button click functionality case. It also adds twig function which helps to generate urls in twig which will act according to the back-url
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	#12990
How to test?	Go to "admin-dev/index.php/sell/customers/1/view" (if don't have customer with id 1 select another). Click on large edit pencil which is located in the first block - it will redirect you to page "admin-dev/index.php/sell/customers/1/edit?" and in that page the "back-url" will exist in the url parameters. Click "save" button - it will point you back to original view page "admin-dev/index.php/sell/customers/1/view" with success message attached. Also repeat the same process and instead of clicking "save", click "cancel" - it will go back to the same link "admin-dev/index.php/sell/customers/1/view"

Write unit tests
Write documentation.

This change is

tomas862 · 2019-04-08T10:55:24Z

@matks its more likely like POC here. Can you have a quick look to it and let men know if I shall finalize it with tests?

src/PrestaShopBundle/Twig/Extension/PathWithBackUrlExtension.php

src/PrestaShopBundle/EventListener/BackUrlRedirectResponseListener.php

matks · 2019-04-08T13:04:36Z

🎉 very nice way to use Symfony built-in features to handle this usecase 😄

src/PrestaShopBundle/EventListener/BackUrlRedirectResponseListener.php

src/PrestaShopBundle/Twig/Extension/PathWithBackUrlExtension.php

tomas862 · 2019-04-15T07:36:34Z

done - waiting for response in #13233 (comment). After the response, I'll re-apply tests

matks · 2019-04-15T08:22:12Z

src/Core/Util/Url/BackUrlProvider.php

+    /**
+     * @param RequestStack|null $requestStack
+     */
+    public function __construct($requestStack)


Let's avoid injecting the RequestStack in services if we can 😉and here we can ! (see https://igor.io/2013/03/31/stateless-services.html for detailed explanation about this matter)

You can pass the request in public function getBackUrl(Request $request).

2 side-effects that are great benefits (apart from the benefit number one which is "do not depend on request stack"):

we can use this service in CLI !

we can use this service on multiple Requests to retrieve the back parameter, not only the one provided by RequestStack

great article! Indeed it was easy to avoid RequestStack in that service - got reduced its usage and right now it is used in PathWithBackUrlExtension twig extension only.

… if current request has back url - it modifies current request and returns copied original request with different url

…t and response objects - e.g "save and stay" functionality

…is being injected

…t param

…rate service

…o confirm the functionality of the redirect response listener

tomas862 · 2019-04-16T08:22:28Z

@matks - let me know if this PR can be approved. Once I know its good to go I'll add docs about it :)

matks · 2019-04-16T08:55:12Z

src/PrestaShopBundle/Resources/config/services/bundle/twig.yml

+      arguments:
+        - '@twig.extension.routing'
+        - '@prestashop.core.uti.back_url_provider'
+        - '@=service("request_stack") ? service("request_stack") : null'


I did not know you could do that 🤔

Yes, that's really handy if you have "optional" dependencies, like from different libraries/bundles

matks · 2019-04-16T08:57:01Z

matks · 2019-04-16T08:58:13Z

@tomas862 just one question: why did you change the name of the GET parameter from "back" to "back-url" ?
If we keep it to "back", this will help for backward compatibility, will it not ? 🤔

tomas862 · 2019-04-16T09:56:39Z

@matks well "back-url" made more sence for me since for word "back" has a greater chance that it might be used in a different context.

and since it is applied only for migrated pages "backward compatability" issue does not exist - the only issue that I see is that when someone will be migrating the pages they should not forget to add "back-url" instead of using "back".

Howerer, using "back" or "back-url" does not make much impact so I can change to "back" again easily if you see its more proper :)

matks · 2019-04-17T12:30:01Z

and since it is applied only for migrated pages "backward compatability" issue does not exist

I was thinking of people using getAdminLink(): since this is able to redirect old legacy URLs to SF urls, then a link with back parameter will still work, wouldn't it ?

…rators working

tomas862 · 2019-04-19T07:19:47Z

@matks you're right - there would be a chance if someone is using getAdminLink with back url parameter to loose this functionality. I just renamed back-url to back :)

PierreRambaud · 2019-04-26T12:07:28Z

Thanks @tomas862

prestonBot added develop Branch Bug Type: Bug labels Apr 8, 2019

tomas862 commented Apr 8, 2019

View reviewed changes

src/PrestaShopBundle/Twig/Extension/PathWithBackUrlExtension.php Outdated Show resolved Hide resolved

tomas862 commented Apr 8, 2019

View reviewed changes

src/PrestaShopBundle/EventListener/BackUrlRedirectResponseListener.php Outdated Show resolved Hide resolved

tomas862 marked this pull request as ready for review April 8, 2019 14:41

tomas862 changed the title ~~[WIP] Enable 'back' GET parameter management~~ Enable 'back' GET parameter management Apr 8, 2019

PierreRambaud reviewed Apr 8, 2019

View reviewed changes

src/PrestaShopBundle/EventListener/BackUrlRedirectResponseListener.php Show resolved Hide resolved

PierreRambaud reviewed Apr 8, 2019

View reviewed changes

src/PrestaShopBundle/Twig/Extension/PathWithBackUrlExtension.php Outdated Show resolved Hide resolved

tomas862 changed the base branch from develop to 1.7.6.x April 9, 2019 06:33

matks reviewed Apr 15, 2019

View reviewed changes

tomas862 added 14 commits April 16, 2019 11:15

adds response listener which listens for redirect response and checks…

316b64b

… if current request has back url - it modifies current request and returns copied original request with different url

adds more specific name for back url "back-url" to query parameters

e229eb7

prevents back functionality if the same url is being called in reques…

18a1e75

…t and response objects - e.g "save and stay" functionality

adds final class and changes some comments

0693b51

renames class

7fed716

adds twig extension which handles back url functionality

afa2844

adds some docs for twig extension

e39273b

instead of extenging routing twig extension instead routingextension …

628d037

…is being injected

adds tests for path with back url extension, removes mandatory reques…

b39e7c1

…t param

adds tests for back url redirect response listener

f0e41aa

adds back-url provider which gets back url from request stack in sepa…

3d44e6b

…rate service

removes request stack usage from back url provider, adds some tests t…

b6a8919

…o confirm the functionality of the redirect response listener

fixes tests for back url retrieval using twig extension

7ef291e

adds back url provider tests

c31f1d7

tomas862 force-pushed the bugfixes/back-url-handling branch from 03e5e6a to c31f1d7 Compare April 16, 2019 08:21

applies php cs fixer

0b321e4

matks reviewed Apr 16, 2019

View reviewed changes

adds "back" instead of "back-ul" keyword to keep the legacy link gene…

9aadca2

…rators working

tomas862 requested a review from a team as a code owner April 19, 2019 07:17

matks approved these changes Apr 19, 2019

View reviewed changes

matks added the Waiting for QA Status: action required, waiting for test feedback label Apr 23, 2019

matks added this to the 1.7.6.0 milestone Apr 23, 2019

marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 24, 2019

PierreRambaud approved these changes Apr 26, 2019

View reviewed changes

PierreRambaud merged commit b11debb into PrestaShop:1.7.6.x Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable 'back' GET parameter management #13233

Enable 'back' GET parameter management #13233

tomas862 commented Apr 8, 2019 •

edited

tomas862 commented Apr 8, 2019

matks commented Apr 8, 2019

tomas862 commented Apr 15, 2019

matks Apr 15, 2019

tomas862 Apr 16, 2019

tomas862 commented Apr 16, 2019

matks Apr 16, 2019

jolelievre Apr 26, 2019

matks commented Apr 16, 2019

matks commented Apr 16, 2019

tomas862 commented Apr 16, 2019 •

edited

matks commented Apr 17, 2019

tomas862 commented Apr 19, 2019

PierreRambaud commented Apr 26, 2019

Enable 'back' GET parameter management #13233

Enable 'back' GET parameter management #13233

Conversation

tomas862 commented Apr 8, 2019 • edited

tomas862 commented Apr 8, 2019

matks commented Apr 8, 2019

tomas862 commented Apr 15, 2019

matks Apr 15, 2019

Choose a reason for hiding this comment

tomas862 Apr 16, 2019

Choose a reason for hiding this comment

tomas862 commented Apr 16, 2019

matks Apr 16, 2019

Choose a reason for hiding this comment

jolelievre Apr 26, 2019

Choose a reason for hiding this comment

matks commented Apr 16, 2019

matks commented Apr 16, 2019

tomas862 commented Apr 16, 2019 • edited

matks commented Apr 17, 2019

tomas862 commented Apr 19, 2019

PierreRambaud commented Apr 26, 2019

tomas862 commented Apr 8, 2019 •

edited

tomas862 commented Apr 16, 2019 •

edited