[BrowserKit] add a way to switch to ajax for one request #24778

Simperfit · 2017-11-01T06:26:57Z

Q	A
Branch?	4.1
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20306
License	MIT
Doc PR	will do

Follow the work on #20306.

/cc @fabpot is it the right implementation ?

linaori · 2017-11-01T07:49:56Z

src/Symfony/Component/BrowserKit/Client.php

@@ -150,6 +150,16 @@ public function getServerParameter($key, $default = '')
        return isset($this->server[$key]) ? $this->server[$key] : $default;
    }

+    public function switchToAjax()


Wouldn't it make more sense to use the term XmlHttpRequest instead of Ajax?

I guess it would make more sense

nicolas-grekas · 2017-11-01T17:27:02Z

src/Symfony/Component/BrowserKit/Client.php

@@ -354,7 +374,7 @@ protected function doRequestInProcess($request)
            unlink($deprecationsFile);
            foreach ($deprecations ? unserialize($deprecations) : array() as $deprecation) {
                if ($deprecation[0]) {
-                    trigger_error($deprecation[1], E_USER_DEPRECATED);
+                    @trigger_error($deprecation[1], E_USER_DEPRECATED);


this change is adding a bug, fabbot is not always right

will revert

nicolas-grekas · 2017-11-01T17:27:36Z

src/Symfony/Component/BrowserKit/Client.php

     *
     * @return Crawler
     */
-    public function submit(Form $form, array $values = array())
+    public function submit(Form $form, array $values = array(), $isAjax = false)


adding arguments to a public method is a BC break, see https://symfony.com/bc

I understood. I have changed the implementation.

dmaicher · 2017-11-01T21:38:00Z

src/Symfony/Component/BrowserKit/Client.php

@@ -150,6 +150,16 @@ public function getServerParameter($key, $default = '')
        return isset($this->server[$key]) ? $this->server[$key] : $default;
    }

+    public function switchToXMLHttpRequest()
+    {
+        $this->setServerParameter('HTTP_X-Requested-With', 'XMLHttpRequest');


shouldn't it be 'HTTP_X_REQUESTED_WITH'?

The header name is really X-Requested-With, so no I guess.

I think @dmaicher is right: PHP normalizes dash to low dashes when hydrating the $_SERVER parameter.

fixed, i've tested it and @dmaicher is actually right, sorry I did just check the header name.

Simperfit · 2017-11-05T16:35:08Z

fabbot failure seems unrelated

Simperfit · 2017-12-05T18:09:45Z

This one is ready for review.

dmaicher · 2017-12-06T07:45:29Z

src/Symfony/Component/BrowserKit/Tests/ClientTest.php

+        $client->request('GET', 'http://example.com/', array(), array(), array(), null, true, true);
+        $this->assertEquals($client->getRequest()->getServer()['HTTP_X_REQUESTED_WITH'], 'XMLHttpRequest');
+        $client->removeXMLHttpRequest();
+        $this->assertEquals('http://example.com/', $client->getRequest()->getUri(), '->getCrawler() returns the Request of the last request');


This should make sure the server param is not set anymore instead?

dmaicher · 2017-12-06T07:48:00Z

src/Symfony/Component/BrowserKit/Client.php

+        $this->setServerParameter('HTTP_X_REQUESTED_WITH', 'XMLHttpRequest');
+    }
+
+    public function removeXMLHttpRequest()


I find this name a bit confusing. We are not removing any request :)

Maybe we can just only have a setter instead? setXMLHttpRequest($isXMLHttpRequest)

I will add Header a the end I think

Simperfit · 2017-12-09T16:06:52Z

Named changed and test added.

fabbot error is wrong.

Simperfit · 2017-12-16T04:54:27Z

AppVeyor failure is unrelated and fabbot error is a false positive.

@fabpot WDYT of this new implem ?

Simperfit · 2018-02-16T09:20:01Z

PR rebased with master

stof · 2018-02-16T09:25:50Z

src/Symfony/Component/BrowserKit/Tests/ClientTest.php

+        $this->assertEquals($client->getRequest()->getServer()['HTTP_X_REQUESTED_WITH'], 'XMLHttpRequest');
+        $client->removeXMLHttpRequestHeader();
+        $this->assertFalse($client->getServerParameter('HTTP_X_REQUESTED_WITH', false));
+        $this->assertEquals('http://example.com/', $client->getRequest()->getUri(), '->getCrawler() returns the Request of the last request');


this last assertion looks useless

that's right, i've removed it.

fabpot · 2018-02-19T07:32:11Z

src/Symfony/Component/BrowserKit/Client.php

@@ -150,6 +150,16 @@ public function getServerParameter($key, $default = '')
        return isset($this->server[$key]) ? $this->server[$key] : $default;
    }

+    public function switchToXMLHttpRequest()


What about switchToXHR instead?

fabpot · 2018-02-19T20:10:24Z

Thank you @Simperfit.

@fabpot

…st (Simperfit) This PR was merged into the 4.1-dev branch. Discussion ---------- [BrowserKit] add a way to switch to ajax for one request | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20306 | License | MIT | Doc PR | will do Follow the work on #20306. /cc @fabpot is it the right implementation ? Commits ------- a10eae7 [BrowserKit] add a way to switch to ajax for one request

javiereguiluz · 2018-03-02T10:06:07Z

I know ... it's too late to talk about this because it's merged ... but I wanted to say that I don't like the current implementation :( It modifies a global estate with switchToXHR() and you must remember later to reset the original estate with removeXHR().

As an alternative, I wonder if we could introduce a new xhrRequest() method that wraps the request() method and it works like it, but does the XHR stuff internally just for that request. Thanks!

Simperfit · 2018-03-02T10:16:50Z

@javiereguiluz I agree with this statement, however I think we should let both switchToXHR and removeXHR public, it can be useful to be able to do it by ourself.

@fabpot WDYT ?

fabpot · 2018-03-02T13:13:55Z

I totally agree with @javiereguiluz here. I was too fast to merge. The description of the PR si misleading.

@Simperfit Can you submit a PR to implement what's on the tin?

Simperfit · 2018-03-02T13:34:08Z

Yes I will be doing it today ! Le ven. 2 mars 2018 à 14:14, Fabien Potencier <notifications@github.com> a écrit :

…

I totally agree with @javiereguiluz <https://github.com/javiereguiluz> here. I was too fast to merge. The description of the PR si misleading. @Simperfit <https://github.com/simperfit> Can you submit a PR to implement what's on the tin? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24778 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8heSVCdp_GWHEUkllXNnl0jYxhNzks5taUWkgaJpZM4QNzPO> .

…pRequest() (Simperfit) This PR was merged into the 4.1-dev branch. Discussion ---------- Transform both switchToXHR() and removeXhr() to xmlHttpRequest() | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | none  | License | MIT | Doc PR | Will do. See symfony/symfony#24778 (comment) for more information about this. We are switching from a possible global estate change to just only one request affected. Commits ------- 4ed08896fa feature: transform both switchToXHR and removeXhr to a xhrRequest

…pRequest() (Simperfit) This PR was merged into the 4.1-dev branch. Discussion ---------- Transform both switchToXHR() and removeXhr() to xmlHttpRequest() | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | none  | License | MIT | Doc PR | Will do. See #24778 (comment) for more information about this. We are switching from a possible global estate change to just only one request affected. Commits ------- 4ed0889 feature: transform both switchToXHR and removeXhr to a xhrRequest

carsonbot added Status: Needs Review BrowserKit Feature labels Nov 1, 2017

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from ebd9e48 to a2b7fe1 Compare November 1, 2017 06:28

linaori reviewed Nov 1, 2017

View reviewed changes

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from a2b7fe1 to 5d2f196 Compare November 1, 2017 08:09

nicolas-grekas reviewed Nov 1, 2017

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Nov 1, 2017

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch 2 times, most recently from 61ae5dc to 512a02e Compare November 1, 2017 17:35

dmaicher reviewed Nov 1, 2017

View reviewed changes

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from 512a02e to 317cdf3 Compare December 1, 2017 12:11

dmaicher reviewed Dec 6, 2017

View reviewed changes

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from 317cdf3 to 7f79a7f Compare December 9, 2017 16:06

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from 7f79a7f to 6c4f0b8 Compare February 16, 2018 09:19

stof reviewed Feb 16, 2018

View reviewed changes

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from 6c4f0b8 to 1a3bf19 Compare February 16, 2018 12:43

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 19, 2018

fabpot requested changes Feb 19, 2018

View reviewed changes

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from 1a3bf19 to 02b1e69 Compare February 19, 2018 14:51

[BrowserKit] add a way to switch to ajax for one request

a10eae7

Simperfit force-pushed the feature/add-a-way-to-switch-to-ajax-for-one-request branch from 02b1e69 to a10eae7 Compare February 19, 2018 14:52

carsonbot removed the Status: Needs Work label Feb 19, 2018

carsonbot added the Status: Reviewed label Feb 19, 2018

fabpot approved these changes Feb 19, 2018

View reviewed changes

fabpot merged commit a10eae7 into symfony:master Feb 19, 2018

javiereguiluz mentioned this pull request Mar 2, 2018

[BrowserKit] add a way to switch to ajax for one request symfony/symfony-docs#9381

Closed

Simperfit deleted the feature/add-a-way-to-switch-to-ajax-for-one-request branch March 2, 2018 10:16

Simperfit mentioned this pull request Mar 2, 2018

Transform both switchToXHR() and removeXhr() to xmlHttpRequest() #26381

Merged

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BrowserKit] add a way to switch to ajax for one request #24778

[BrowserKit] add a way to switch to ajax for one request #24778

Simperfit commented Nov 1, 2017 •

edited

Loading

linaori Nov 1, 2017

Simperfit Nov 1, 2017

nicolas-grekas Nov 1, 2017

Simperfit Nov 1, 2017

nicolas-grekas Nov 1, 2017

Simperfit Nov 1, 2017

dmaicher Nov 1, 2017

Simperfit Nov 4, 2017

dunglas Nov 30, 2017

Simperfit Dec 1, 2017

Simperfit commented Nov 5, 2017 •

edited

Loading

Simperfit commented Dec 5, 2017

dmaicher Dec 6, 2017

dmaicher Dec 6, 2017

Simperfit Dec 8, 2017

Simperfit commented Dec 9, 2017

Simperfit commented Dec 16, 2017

Simperfit commented Feb 16, 2018

stof Feb 16, 2018

Simperfit Feb 16, 2018

fabpot Feb 19, 2018

Simperfit Feb 19, 2018

fabpot commented Feb 19, 2018

javiereguiluz commented Mar 2, 2018

Simperfit commented Mar 2, 2018

fabpot commented Mar 2, 2018

Simperfit commented Mar 2, 2018 via email

[BrowserKit] add a way to switch to ajax for one request #24778

[BrowserKit] add a way to switch to ajax for one request #24778

Conversation

Simperfit commented Nov 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Nov 5, 2017 • edited Loading

Simperfit commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Dec 9, 2017

Simperfit commented Dec 16, 2017

Simperfit commented Feb 16, 2018

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

fabpot commented Feb 19, 2018

javiereguiluz commented Mar 2, 2018

Simperfit commented Mar 2, 2018

fabpot commented Mar 2, 2018

Simperfit commented Mar 2, 2018 via email

Simperfit commented Nov 1, 2017 •

edited

Loading

Simperfit commented Nov 5, 2017 •

edited

Loading