Fixed the issue with new selenium v2.35 session id retrieval handshake #266
Conversation
|
Great the fix works! verified with selenium v2.35 on mac |
|
Looks like Giorgio is deadlocked in distributed transactions, but we really need a merge here. |
|
@julianseeger pinged him in twitter https://twitter.com/zerkms/status/369412273686597633 :-) |
| $sessionId = $this->jsonResponse['sessionId']; | ||
|
|
||
| // if url doesn't have sessionId included - append it manually | ||
| // this change was performed in selenium v2.35 |
| // @see https://code.google.com/p/selenium/issues/detail?id=6089 | ||
| // @see https://github.com/sebastianbergmann/phpunit-selenium/issues/265 | ||
| if (strpos($url, $sessionId) === false) { | ||
| $url = trim($url, '/') . '/' . $sessionId; |
There was a problem hiding this comment.
i have been checked this and i have seen that the $this->info['url'] is not including the ending /, you can check also
There was a problem hiding this comment.
@felixcarmona yep, it doesn't at the moment, but I don't see any guarantee that JsonWireProtocol provides about it
There was a problem hiding this comment.
@zerkms, so, should sanitize it always no?, not only in this if statement no?
i think this trim (or rtrim) should be done when the $url is setted at the first time:
$url = rtrim($this->info['url'], '/');
| // @see https://code.google.com/p/selenium/issues/detail?id=6089 | ||
| // @see https://github.com/sebastianbergmann/phpunit-selenium/issues/265 | ||
| if (strpos($url, $sessionId) === false) { | ||
| $url = rtrim($url, '/') . '/' . $sessionId; |
There was a problem hiding this comment.
why not rtrim the $url when the $url contains the $sessionId?
why not do the rtrim when you set the $url at the first time?
$url = rtrim($this->info['url'], '/');
There was a problem hiding this comment.
@felixcarmona the aim of this patch is to bring phpunit-selenium back to work, so I implemented the fix in the most clear and obvious way I see it :-)
There was a problem hiding this comment.
so, the url sanitize in the if statement (with rtrim) is also out of the aim of the patch proposal, the url should be sanitized at the two ways or at nothing
|
👍 |
|
@felixcarmona finally I could satisfy your demands :-) |
Fixed the issue with new selenium v2.35 session id retrieval handshake
|
Do we need a new release right now or master is fine? |
|
master seem fine, because this change respects the retrocompatibility |
A fix for #265