Restore request in 2.3-dev doesn't work correctly

5 years ago

o5
Member | 417
+
0
-

Hello,

I think in @dev version is a bug with method $presenter->restoreRequest().

Prepare reproducible environment:

git clone git@github.com:nette/examples.git
cd examples/CD-collection/
composer install --prefer-dist

User story:

  1. Sign in into app
  2. Go to “Add new album” screen
  3. Delete all cookies via Developers Tools
  4. Refresh page
  5. You have been redirect to login form with backlink param
  6. After successfully re-sign in, you're in “Add new album” screen, but URI wasn't change and if you do refresh page, you get “Confirm Form Resubmission”.

The same story works fine (without form resubmission) in Nette v2.2.2.

Last edited by o5 (2014-06-27 13:02)

5 years ago

Majkl578
Moderator | 1379
+
0
-

Probably because of this from @enumag.
See discussion below original commit here.

5 years ago

enumag
Member | 2128
+
0
-

This behaviour is correct.

The method restoreRequest only restores the request without changing url (it can't actually change it because restoring post requests wouldn't work). The fact you got redirected before was an unintentional sideeffect caused by canonicalization in some cases (not always so it was not reliable).

We should add some method redirectToStoredRequest() or something like that which would ignore the post data in the stored request and redirect.

Last edited by enumag (2014-06-27 18:46)

5 years ago

Majkl578
Moderator | 1379
+
+1
-

Ignoring the fact it is masssive BC break of common use-case of most websites (and thus as well violates POST/REDIRECT/GET pattern), how do you suggest to replace the behavior? Calling restoreRequest() terminates code execution, so you can't place redirect after it. And any other way is imho hackish.

5 years ago

enumag
Member | 2128
+
-1
-

I want to introduce a new method which should be used instead of restoreRequest if the redirect is desired.

5 years ago

enumag
Member | 2128
+
-2
-

See my solution here and here.

5 years ago

duke
Member | 650
+
+1
-

I am currently solving this problem by this code in my BasePresenter:

protected function startup()
{
    parent::startup();
    if ($this->request->hasFlag(\Nette\Application\Request::RESTORED)) {
        $this->redirect('this');
    }
}

5 years ago

o5
Member | 417
+
0
-

@duke: Thanks, I'll use your solution.

@enumag: I want to see your explanation for people after push this BC into stable version.

Last edited by o5 (2014-06-28 23:49)

5 years ago

enumag
Member | 2128
+
0
-

I've already explained reasons for this change on GitHub. As I've said the old behavior (although relied on by many people) was neither intended nor reliable, it was just a side-effect of other things.

Also it's not my fault that this change got into Nette 2.2. At the time I was convinced that the next version after 2.1 will be 3.0 where such BC break would be fine.

Last edited by enumag (2014-06-28 22:19)

5 years ago

Majkl578
Moderator | 1379
+
0
-

old behavior (although relied on by many people) was neither intended nor reliable, it was just a side-effect of other things

I think it was perfectly reliable and handsome.

Also it's not my fault that this change got into Nette 2.2.

It didn't.

5 years ago

enumag
Member | 2128
+
0
-

Oh, didn't notice that the bug report was for 2.3-dev only. :-) In that case it's fine. When you're using master, things like this should be expected.

Last edited by enumag (2014-06-29 10:24)

5 years ago

David Grudl
Nette Core | 6877
+
+1
-

This will be solved (ie. replaced in master) by https://github.com/…ation/pull/4