Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw explanative LogicException when driver is not started yet #268

Closed
wants to merge 2 commits into from

Conversation

rvanlaak
Copy link
Contributor

@rvanlaak rvanlaak commented Dec 11, 2019

When executeScript gets called before the client made a request, an exception should get thrown. The problem; the actual RuntimeException can not get created because driver is null.

This PR clarifies that these methods can not get called before the driver is started.

Failing test on linting / phpstan does not seem related (?). Is it because of the 0.12 phpstan release of several days ago?

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but we need to fix the tests before merging (in another PR).

@@ -17,7 +17,8 @@
use Facebook\WebDriver\JavaScriptExecutor;
use Facebook\WebDriver\WebDriver;
use Facebook\WebDriver\WebDriverExpectedCondition;
use Symfony\Component\BrowserKit\Client as BrowserKitClient;
use LogicException;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use \LogicException instead of this in Symfony.

Copy link
Contributor Author

@rvanlaak rvanlaak Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, you mean not importing the exception class? Ok, did change that in ea9d7d1.

@rvanlaak
Copy link
Contributor Author

LGTM, but we need to fix the tests before merging (in another PR).

This PR actually fixes CI for PHP 7.2 , which is failing on master. The failing has to do with PHPStan: https://travis-ci.org/symfony/panther/jobs/626071903?utm_medium=notification&utm_source=github_status

@dunglas would you be ok with me adding PHPStan in a separate PR and run it via Github Actions? See php-translation/symfony-storage#38 and theiconic/name-parser#41 and rvanlaak/SettingsBundle#110 as examples 👍

@dunglas dunglas added the duplicate This issue or pull request already exists label Oct 2, 2020
@dunglas dunglas closed this in #373 Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants