Added getOutput method for full output data access on any request #780
Conversation
|
The failing tests are because return type static is not supported in php 7.4. If this lib is to support php 7.4, the return type static must be removed. Otherwise, composer.json needs to be changed to point to 8.0 and up. |
|
Don't commit code formatting changes in your PR. It's hard to see what's new. I'm trying to see if your PR could be merged instead of mine #779 to fix the large file issue. |
vitorsemeano
force-pushed
the
full_output_data
branch
from
0606c2c to
02e6fe1
Compare
Sorry for the late reply. Already reverted style changes and updated with upstream. |
minor fixes for PHPDoc
src/Browsershot.php
Outdated
| return rtrim($process->getOutput()); | ||
| $rawOutput = rtrim($process->getOutput()); | ||
|
|
||
| $this->output = json_decode($rawOutput, true); |
There was a problem hiding this comment.
This output is now an array. Let's make that a bit more structured.
Could you add a class ChromiumResult that takes that array as a constructor argument?
The class should have methods such as pageErrors(), so that stuff like
$this->output['pageErrors'] ?? null
can be replaced with (that null check can be done inside of pageErrors() itself.
$this->output->pageErrors();
Let's also rename $this->output to $this->chromiumResult
Maybe also
There was a problem hiding this comment.
Created object as suggested.
src/ChromiumResult.php
Outdated
| $this->redirectHistory = $output['redirectHistory'] ?? null; | ||
| } | ||
|
|
||
| public function getResult() |
There was a problem hiding this comment.
Could you add return types for all these functions?
src/Browsershot.php
Outdated
| /** | ||
| * @var ChromiumResult|null | ||
| */ | ||
| private $chromiumResult = null; |
There was a problem hiding this comment.
Make this protected. Let's also type the property, and all other properties. Feel free to bump the required PHP version in composer.json if needed.
There was a problem hiding this comment.
Done. I think there's no need to bump php version even further.
|
Thank you! |
When errors occur on any method invoked by browsershot to the puppeteer browser, it's not easy to access console log data right away. Other methods exist like consoleMessages that allows access to these logs on a new browser call, not getting the original logs that were generated on the moment the first call was made.
To facilitate access to these logs, I developed the method getOuput, providing full data access after any call made to the browser.
Instead of only returning the pretended content in each browser call, I modified the output to return a standard object containing the following data:
consoleMessages: any console messages generated during browser execution
requestsList: all endpoints called by browser
failedRequests: all endpoints that failed
result: result of the method call to the browser. In case of exception this is not set
exception: in case of a exception, the string representation of it will be here. In success this is not set
pageErrors: all page errors generated for the current command. This will show any javascript error.
Internally this object is decomposed into the real required data by any browsershot method, and then the full object is stored and accessible using getOutput method.
If any additional call to the browser is required, the current output data will be erased and the new one will be placed accordingly.
I made two new tests, calling simple methods of browsershot, and analysing the getOutput structure in two possible situations: success and exception thrown.
To complement this output, I added the method pageErrors, following the same logic for the other type of info that you can get using browsershot api.
This is the same PR as #730, updated with upstream and with page error info.
I would appreciate some feedback in this approach.