New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 19, 2018

Conversation

@Simperfit
Contributor

Simperfit commented Nov 1, 2017

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 ?

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

This comment has been minimized.

@iltar

iltar Nov 1, 2017

Contributor

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

This comment has been minimized.

@Simperfit

Simperfit Nov 1, 2017

Contributor

I guess it would make more sense

@@ -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 comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@Simperfit

Simperfit Nov 1, 2017

Contributor

will revert

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@Simperfit

Simperfit Nov 1, 2017

Contributor

I understood. I have changed the implementation.

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

@@ -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');

This comment has been minimized.

@dmaicher

dmaicher Nov 1, 2017

Contributor

shouldn't it be 'HTTP_X_REQUESTED_WITH'?

This comment has been minimized.

@Simperfit

Simperfit Nov 4, 2017

Contributor

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

This comment has been minimized.

@dunglas

dunglas Nov 30, 2017

Member

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

This comment has been minimized.

@Simperfit

Simperfit Dec 1, 2017

Contributor

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

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 5, 2017

fabbot failure seems unrelated

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 5, 2017

This one is ready for review.

$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 comment has been minimized.

@dmaicher

dmaicher Dec 6, 2017

Contributor

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

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

This comment has been minimized.

@dmaicher

dmaicher Dec 6, 2017

Contributor

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

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

This comment has been minimized.

@Simperfit

Simperfit Dec 8, 2017

Contributor

I will add Header a the end I think

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 9, 2017

Named changed and test added.

fabbot error is wrong.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 16, 2017

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

@fabpot WDYT of this new implem ?

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Feb 16, 2018

PR rebased with master

$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 comment has been minimized.

@stof

stof Feb 16, 2018

Member

this last assertion looks useless

This comment has been minimized.

@Simperfit

Simperfit Feb 16, 2018

Contributor

that's right, i've removed it.

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

This comment has been minimized.

@fabpot

fabpot Feb 19, 2018

Member

What about switchToXHR instead?

This comment has been minimized.

@Simperfit

Simperfit Feb 19, 2018

Contributor

done

@fabpot

fabpot approved these changes Feb 19, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 19, 2018

Thank you @Simperfit.

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

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Feb 19, 2018

feature #24778 [BrowserKit] add a way to switch to ajax for one reque…
…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

This comment has been minimized.

Member

javiereguiluz commented Mar 2, 2018

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

This comment has been minimized.

Contributor

Simperfit commented Mar 2, 2018

@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 ?

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

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 2, 2018

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

This comment has been minimized.

Contributor

Simperfit commented Mar 2, 2018

symfony-splitter pushed a commit to symfony/browser-kit that referenced this pull request Mar 20, 2018

feature #26381 Transform both switchToXHR() and removeXhr() to xmlHtt…
…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 <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none   <!-- #-prefixed issue number(s), if any -->
| 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

fabpot added a commit that referenced this pull request Mar 20, 2018

feature #26381 Transform both switchToXHR() and removeXhr() to xmlHtt…
…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 <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none   <!-- #-prefixed issue number(s), if any -->
| 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

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment