Allow using an env var to determine chrome driver binary #49
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dunglas
requested changes
Jul 9, 2018
Member
dunglas
left a comment
dunglas
left a comment
There was a problem hiding this comment.
Can you also document this environment variable in the README please?
| public function __construct(?string $chromeDriverBinary = null, ?array $arguments = null, array $options = []) | ||
| { | ||
| $this->process = new Process([$chromeDriverBinary ?? $this->findChromeDriverBinary()], null, null, null, null); | ||
| $this->process = new Process([$chromeDriverBinary ?: $this->findChromeDriverBinary()], null, null, null, null); |
Contributor
Author
There was a problem hiding this comment.
To avoid $chromeDriverBinary be an empty string 😄
Contributor
Author
There was a problem hiding this comment.
With a simple developer mistake, I know I might be paranoid, but if you think I should revert this, feel free to ask, and I will
Member
There was a problem hiding this comment.
No it's ok! Can you just document this new env var in the README file please?
src/ProcessManager/ChromeManager.php
Outdated
|
|
||
| private function findChromeDriverBinary(): string | ||
| { | ||
| if (false !== $binary = \getenv('PANTHERE_CHROME_DRIVER_BINARY')) { |
Member
There was a problem hiding this comment.
we use $_ENV in other places (getenv isn't thread safe).
dunglas
approved these changes
Jul 9, 2018
Member
|
Thanks @Pierstoval |
dunglas
pushed a commit
that referenced
this pull request
Dec 28, 2020
* Allow using an env var to determine chrome driver binary * Added small documentation to use the PANTHERE_CHROME_DRIVER_BINARY env var
nicolas-grekas
pushed a commit
that referenced
this pull request
Jan 18, 2021
* Allow using an env var to determine chrome driver binary * Added small documentation to use the PANTHERE_CHROME_DRIVER_BINARY env var
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My problem is that Panthère doesn't work on Alpine (because on Alpine we should either use the driver coming from the repository, or recompile it manually).
For this, it could be nice to tell Alpine users to use either the package or to recompile it instead of using Panthère's provided binary. And in general, to get the latest version, it's better to propose them to download the package in another way, for them to be sure 😅.
For now, an env var is the best solution to use another binary, to me.
WDYT?