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

#446 Check for session first before logging out in fastLogout method of DrupalAuthenticationManager #447

Merged
merged 3 commits into from
Dec 15, 2017

Conversation

m4olivei
Copy link
Contributor

Attempt at resolving #446.

@m4olivei
Copy link
Contributor Author

This patch works for me. I think it's a Selenium specific issue, where the Selenium Mink Driver is assuming that the session is already started when you call reset here: https://github.com/minkphp/MinkSelenium2Driver/blob/v1.3.1/src/Selenium2Driver.php#L359. When it's not, 💥 .

@m4olivei m4olivei mentioned this pull request Dec 13, 2017
@m4olivei
Copy link
Contributor Author

Incidentally, I see there is some awesome test coverage in this package. I wonder if it would be possible to have some test cases with Mink Selenium2 Driver and a browser like Chrome?

@jhedstrom
Copy link
Owner

Incidentally, I see there is some awesome test coverage in this package. I wonder if it would be possible to have some test cases with Mink Selenium2 Driver and a browser like Chrome?

That would be great! Happy to review PRs that update .travis.yml to add some coverage there (and perhaps a new suite or profile in behat.yml.dist to specifically target selenium...

$session = $this->getSession();
if ($session->isStarted()) {
$session->reset();
$this->userManager->setCurrentUser(false);
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should unset the user outside the conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, will update.

@m4olivei
Copy link
Contributor Author

m4olivei commented Dec 14, 2017

That would be great! Happy to review PRs that update .travis.yml to add some coverage there (and perhaps a new suite or profile in behat.yml.dist to specifically target selenium...

I took an honest look at it (being new to Travis), and it seems like it would be tricky to get all the dependencies right to be able to run chrome in a window manager and hook selenium up to it. I've seen it done in CircleCI before. Although it would be nice, I'm going to slowly climb out of this rabbit hole for now 😄.

@jhedstrom jhedstrom merged commit 4fd9abc into jhedstrom:master Dec 15, 2017
@jhedstrom
Copy link
Owner

Thanks!

@jhedstrom
Copy link
Owner

@m4olivei I found this Travis setup for the mink selenium driver, it might help you out if you get the chance :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants