Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[BrowserKit] add a way to switch to ajax for one request#24778
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ebd9e48 toa2b7fe1Compare| returnisset($this->server[$key]) ?$this->server[$key] :$default; | ||
| } | ||
| publicfunctionswitchToAjax() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wouldn't it make more sense to use the termXmlHttpRequest instead ofAjax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess it would make more sense
a2b7fe1 to5d2f196Compare| foreach ($deprecations ?unserialize($deprecations) :array()as$deprecation) { | ||
| if ($deprecation[0]) { | ||
| trigger_error($deprecation[1],E_USER_DEPRECATED); | ||
| @trigger_error($deprecation[1],E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this change is adding a bug, fabbot is not always right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
will revert
| * @return Crawler | ||
| */ | ||
| publicfunctionsubmit(Form$form,array$values =array()) | ||
| publicfunctionsubmit(Form$form,array$values =array(),$isAjax =false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
adding arguments to a public method is a BC break, seehttps://symfony.com/bc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I understood. I have changed the implementation.
61ae5dc to512a02eCompare| publicfunctionswitchToXMLHttpRequest() | ||
| { | ||
| $this->setServerParameter('HTTP_X-Requested-With','XMLHttpRequest'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't it be'HTTP_X_REQUESTED_WITH'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The header name is really X-Requested-With, so no I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think@dmaicher is right: PHP normalizes dash to low dashes when hydrating the $_SERVER parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed, i've tested it and@dmaicher is actually right, sorry I did just check the header name.
Simperfit commentedNov 5, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
fabbot failure seems unrelated |
512a02e to317cdf3CompareSimperfit commentedDec 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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should make sure the server param is not set anymore instead?
| $this->setServerParameter('HTTP_X_REQUESTED_WITH','XMLHttpRequest'); | ||
| } | ||
| publicfunctionremoveXMLHttpRequest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I find this name a bit confusing. We are not removing any request :)
Maybe we can just only have a setter instead?setXMLHttpRequest($isXMLHttpRequest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will add Header a the end I think
317cdf3 to7f79a7fCompareSimperfit commentedDec 9, 2017
Named changed and test added. fabbot error is wrong. |
Simperfit commentedDec 16, 2017
AppVeyor failure is unrelated and fabbot error is a false positive. @fabpot WDYT of this new implem ? |
7f79a7f to6c4f0b8CompareSimperfit commentedFeb 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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this last assertion looks useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that's right, i've removed it.
6c4f0b8 to1a3bf19Compare| returnisset($this->server[$key]) ?$this->server[$key] :$default; | ||
| } | ||
| publicfunctionswitchToXMLHttpRequest() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What aboutswitchToXHR instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
1a3bf19 to02b1e69Compare02b1e69 toa10eae7Comparefabpot commentedFeb 19, 2018
Thank you@Simperfit. |
…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 doFollow 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 commentedMar 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 As an alternative, I wonder if we could introduce a new |
Simperfit commentedMar 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 ? |
fabpot commentedMar 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 commentedMar 2, 2018 via email
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 <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://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.Seesymfony/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 <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://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
Uh oh!
There was an error while loading.Please reload this page.
Follow the work on#20306.
/cc@fabpot is it the right implementation ?