Adding support for http endpoint in addition to the websocket #599
Conversation
src/Communication/Connection.php
Outdated
| $parsedUrl = parse_url($socketClient); | ||
| if (isset($parsedUrl['scheme']) && ($parsedUrl['scheme']==='http' or $parsedUrl['scheme']==='https')) |
There was a problem hiding this comment.
The expression could be simpler:
| $parsedUrl = parse_url($socketClient); | |
| if (isset($parsedUrl['scheme']) && ($parsedUrl['scheme']==='http' or $parsedUrl['scheme']==='https')) | |
| if (\in_array(\parse_url($socketClient, PHP_URL_SCHEME), ['http', 'https']) |
There was a problem hiding this comment.
please always use strict mode, unless you have a very good reason to not do it - it's less important in modern php, but in <=7.4: https://3v4l.org/Yc3UU
src/Communication/Connection.php
Outdated
|
|
||
| throw new \Exception("Unable to request $configURL"); | ||
| } | ||
| else { |
There was a problem hiding this comment.
This else isn't doing anything. The throw in the previous if will stop the execution on this method.
src/Communication/Connection.php
Outdated
| $resp = curl_exec($curl); | ||
|
|
||
| if ($resp === false) { | ||
| $err = curl_error($curl); |
There was a problem hiding this comment.
Why get the error if we are not using it for anything?
|
Thanks for the contribution. I left some comments in the code. |
|
Hi @enricodias, thanks a lot for your feedback. I just committed a few changes from your recommendations. |
src/Communication/Connection.php
Outdated
|
|
||
| $configURL = $socketClient.'/json/version'; | ||
|
|
||
| $resp = \file_get_contents($configURL); |
There was a problem hiding this comment.
We can't call this without doing any error handling (yes, I really do mean "error" in the PHP 5 sense of the word).
|
Some environments will prevent file functions from accessing external urls. I could just check if curl is available and use it. |
Hey @enricodias , curl is back and I've added a check for it. |
| @@ -106,6 +106,35 @@ public function __construct($socketClient, LoggerInterface $logger = null, int $ | |||
|
|
|||
| // create socket client | |||
| if (\is_string($socketClient)) { | |||
| if (\in_array(\parse_url($socketClient, \PHP_URL_SCHEME), ['http', 'https'])) { | |||
There was a problem hiding this comment.
Wouldn't it be better to extract all this code into a private/protected method? something like
private function retrieveWsDebuggerUrl(string $endpoint): string
In order to use Chrome Headless you need to know the
webSocketDebuggerUrl.This address can be retrieved by requesting
/json/versionas mentioned at https://chromedevtools.github.io/devtools-protocol/.With this PR the goal is to support both the websocket address and the HTTP endpoint.