[BrowserKit] Emulate back/forward browser navigation #22341

e-moe · 2017-04-07T12:14:20Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22336
License	MIT
Doc PR	CHANGELOG.md updated

Hi all, please review this code for emulating back/forward browser navigation (skip redirects). If code is ok I will add tests and docs

sstok · 2017-04-07T13:48:44Z

As this is a new feature please rebase this on the master and change the pull-request target. Also this is missing some tests.

Status: needs work

e-moe · 2017-04-07T13:53:49Z

@sstok , thanks for your comment. PR target is changed to master.

as for tests - I'm waiting for implementation review/approval. if it's done correctly I will add missing tests

stof · 2017-04-07T14:15:45Z

src/Symfony/Component/BrowserKit/Client.php

@@ -328,6 +340,8 @@ public function request($method, $uri, array $parameters = array(), array $files
        }

        if ($this->followRedirects && $this->redirect) {
+            $this->redirects[] = spl_object_hash($this->history->current());


you should use the hash as key rather than value. the array is used to check whether it contains a given hash, and O(1) is better than than O(n)

stof · 2017-04-07T14:16:15Z

src/Symfony/Component/BrowserKit/Client.php

    protected $followRedirects = true;
+    protected $browserNavigation = false;


please use private visibility for any new property. We don't want to make them extension points yet

stof · 2017-04-07T14:17:20Z

src/Symfony/Component/BrowserKit/Client.php

-        return $this->requestFromRequest($this->history->back(), false);
+        do {
+            $request = $this->history->back();
+        } while ($this->browserNavigation && in_array(spl_object_hash($request), $this->redirects));


what if we reach the beginning of the history ?

History object will throw \LogicException('You are already on the first page.');

the behavior will be the same

javiereguiluz · 2017-04-07T15:19:48Z

I'd like to ask if this is something that browsers do contrary to the standard ... or if this is the behavior proposed by the related standard. That way we'll know if this is a new feature or a bug. Thanks!

e-moe · 2017-04-08T18:09:30Z

@javiereguiluz , maybe we can check this document: https://www.w3.org/TR/html5/browsers.html#history

curry684 · 2017-04-09T09:05:37Z

Assuming the BrowserKit client should conform to that standard it's a bug, as history.back() is defined to:

Goes back one step in the joint session history.

If there is no previous page, does nothing.

Joint session history is defined as (emphasis moved):

The joint session history of a top-level browsing context is the union of all the session histories of all browsing contexts of all the fully active Document objects that share that top-level browsing context, with all the entries that are current entries in their respective session histories removed except for the current entry of the joint session history.

As a redirect is no fully active Document object navigation should bypass it, hence it's a bug.

But also a BC breaking bug as there may be tons of tests out there explicitly going back twice in history to circumvent this behavior.

e-moe · 2017-04-09T11:10:07Z

@curry684 , thanks for you findings! Sine this is a bugfix should I rebase this PR to 2.7 branch?

fabpot · 2017-04-09T14:23:03Z

As it might break existing tests, I think this needs to be merged in master, even if this is a bug fix.

fabpot · 2017-04-09T14:30:05Z

src/Symfony/Component/BrowserKit/Client.php

+     *
+     * @param bool $browserNavigation Whether to emulate browser navigation
+     */
+    public function browserNavigation($browserNavigation = true)


I would not make it configurable as browsers don't make this configurable either. I understand that you probably want to be able to keep BC here. But I don't think that this is a good idea. The current behavior is wrong and we should fix it. What needs to be done though is to add a note in the CHANGELOG/UPGRADE files to document this change.

curry684 · 2017-04-09T15:07:29Z

Given the possibly severe BC breaks I'd also recommend considering this a functionality change, hence master.

e-moe · 2017-04-10T07:43:16Z

Tests and docs added, comments applied.

Also, I changed spl_object_hash to serialize due to clone usage in History object: hashes was new all the time (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/BrowserKit/History.php#L68)

staabm · 2017-04-10T15:48:19Z

src/Symfony/Component/BrowserKit/Client.php

@@ -328,6 +329,8 @@ public function request($method, $uri, array $parameters = array(), array $files
        }

        if ($this->followRedirects && $this->redirect) {
+            $this->redirects[serialize($this->history->current())] = true;


did you consider spl_object_hash instead of serialize? It should be way faster and less memory intensive.

http://php.net/manual/en/function.spl-object-hash.php

See comment above done by @e-moe explaining why it was changed.

willemstuursma · 2017-06-06T13:49:36Z

Anything I can do to make this go forward?

fabpot · 2017-07-06T07:09:31Z

Thank you @e-moe.

…e-moe) This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes #22341). Discussion ---------- [BrowserKit] Emulate back/forward browser navigation | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22336 | License | MIT | Doc PR | CHANGELOG.md updated Hi all, please review this code for emulating back/forward browser navigation (skip redirects). If code is ok I will add tests and docs Commits ------- 680da44 [BrowserKit] Emulate back/forward browser navigation

carsonbot added Status: Needs Review BrowserKit Bug Feature labels Apr 7, 2017

e-moe force-pushed the ticket_22336 branch 3 times, most recently from 637abad to 46b8e7b Compare April 7, 2017 12:30

e-moe changed the title ~~[BrowserKit] don't save redirects in history~~ [BrowserKit] Emulate back/forward browser navigation Apr 7, 2017

e-moe force-pushed the ticket_22336 branch 3 times, most recently from 05f2c01 to 44df302 Compare April 7, 2017 12:56

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 7, 2017

e-moe changed the base branch from 2.7 to master April 7, 2017 13:52

e-moe force-pushed the ticket_22336 branch from 44df302 to 843c184 Compare April 7, 2017 13:52

sstok mentioned this pull request Apr 7, 2017

Browserkit back should not go back to URLs that redirect #22336

Closed

stof reviewed Apr 7, 2017

View reviewed changes

e-moe force-pushed the ticket_22336 branch 2 times, most recently from f321c80 to 4c8af71 Compare April 7, 2017 14:23

fabpot reviewed Apr 9, 2017

View reviewed changes

e-moe force-pushed the ticket_22336 branch from 4c8af71 to 7a28a44 Compare April 10, 2017 06:54

[BrowserKit] Emulate back/forward browser navigation

91dba0e

e-moe force-pushed the ticket_22336 branch from 7a28a44 to 91dba0e Compare April 10, 2017 07:40

nicolas-grekas modified the milestone: 3.4 Apr 10, 2017

staabm reviewed Apr 10, 2017

View reviewed changes

fabpot approved these changes Jul 6, 2017

View reviewed changes

fabpot closed this Jul 6, 2017

This was referenced Jul 9, 2017

BrowserKit back/forward behaviour not consistent with other browsers #20138

Closed

[BrowserKit] Emulate back/forward browser navigation symfony/symfony-docs#8164

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

rvanlaak mentioned this pull request May 23, 2018

PHPUnit tests almost 300% slower after Symfony update #24596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BrowserKit] Emulate back/forward browser navigation #22341

[BrowserKit] Emulate back/forward browser navigation #22341

e-moe commented Apr 7, 2017 •

edited

Loading

sstok commented Apr 7, 2017

e-moe commented Apr 7, 2017 •

edited

Loading

stof Apr 7, 2017

e-moe Apr 7, 2017

stof Apr 7, 2017

e-moe Apr 7, 2017

stof Apr 7, 2017

e-moe Apr 7, 2017

stof Apr 7, 2017

e-moe Apr 7, 2017

javiereguiluz commented Apr 7, 2017

e-moe commented Apr 8, 2017

curry684 commented Apr 9, 2017

e-moe commented Apr 9, 2017

fabpot commented Apr 9, 2017

fabpot Apr 9, 2017

e-moe Apr 10, 2017

curry684 commented Apr 9, 2017

e-moe commented Apr 10, 2017

staabm Apr 10, 2017

stloyd Apr 10, 2017

willemstuursma commented Jun 6, 2017

fabpot commented Jul 6, 2017

		protected $followRedirects = true;
		protected $browserNavigation = false;

[BrowserKit] Emulate back/forward browser navigation #22341

[BrowserKit] Emulate back/forward browser navigation #22341

Conversation

e-moe commented Apr 7, 2017 • edited Loading

sstok commented Apr 7, 2017

e-moe commented Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Apr 7, 2017

e-moe commented Apr 8, 2017

curry684 commented Apr 9, 2017

e-moe commented Apr 9, 2017

fabpot commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curry684 commented Apr 9, 2017

e-moe commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willemstuursma commented Jun 6, 2017

fabpot commented Jul 6, 2017

e-moe commented Apr 7, 2017 •

edited

Loading

e-moe commented Apr 7, 2017 •

edited

Loading