-
Notifications
You must be signed in to change notification settings - Fork 276
Xdebug support in PHPStorm #282
base: master
Are you sure you want to change the base?
Conversation
Tests are failing because of external dependency's, I would drop php 5.3 support. |
features/search.feature
Outdated
@@ -4,13 +4,13 @@ Feature: Search | |||
I need to be able to search for a word | |||
|
|||
Scenario: Searching for a page that does exist | |||
Given I am on "/wiki/Main_Page" | |||
Given I am on "http://en.wikipedia.org/wiki/Main_Page" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you change this? It already set in configuration https://github.com/Behat/MinkExtension/pull/282/files#diff-7bde54db60a6e933518d8b61b929edceR10
You should not use short array syntax to get compatible with php 5.3 |
Hello @spolischook, Changes are made. As for your second comment, there is no short array syntax used anywhere in my commit. |
composer.json
Outdated
"behat/behat": "~3.0,>=3.0.5", | ||
"behat/mink": "~1.5", | ||
"symfony/config": "~2.2|~3.0" | ||
"name": "behat/mink-extension", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't change the indentation of the file
.gitignore
Outdated
@@ -2,3 +2,5 @@ | |||
*.phar | |||
composer.lock | |||
vendor | |||
.idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one must be removed. It is specific to your system, not to the project, so it should not be added in the project-level ignore rules. See https://help.github.com/articles/ignoring-files/ to know how to ignore it on your system
features/src/FeatureContext.php
Outdated
/** | ||
* @Then /^I should have the "([^"]*)" cookie with value "([^"]*)"$/ | ||
*/ | ||
public function iShouldHaveTheCookieWithValue($cookie_name, $cookie_expected_value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please respect our coding standards. We use PSR-2, so variable names are camelCased, and the curly brace should go on the next line (as done for the previous method)
features/src/FeatureContext.php
Outdated
if ($cookie_real_value !== $cookie_expected_value) { | ||
throw new ExpectationException( | ||
'The cookie with name ' . $cookie_name . ' was found, but does not contain ' . $cookie_real_value . ', yet it contains ' . $cookie_expected_value . '.', | ||
$this->getSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing the session in the constructor is deprecated. Please use the new API.
Btw, you could use the Mink assertion API for this: $this->assertSession()->cookieEquals(...)
instead of duplicating its logic
Hello @stof, Thanks for the review, makes sense. Changes have been applied. |
@haringsrob any way you need to fix tests before merge your impruvement https://travis-ci.org/Behat/MinkExtension/jobs/240659573#L350 |
@spolischook I will remove the hhvm. For the other part I don't agree, Also php 5.3 is already EOL for some time, http://php.net/eol.php, keeping on using it might give developer the wrong idea that it is ok. |
@@ -7,11 +7,9 @@ cache: | |||
- $HOME/.composer/cache/files | |||
|
|||
php: | |||
- 5.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the min version in composer.json is still 5.3, this is invalid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the composer.json now. Thanks, let me know if it is not the correct way.
Hi,
This is an initial proposal to add PHPstorm xdebug integration.
Current situation is that, you can debug behat as long as calls are direct, however, once http requests are involved like
When I am on "\"
then the debugger will no longer continue as no cookie is set.This pull requests solves this by checking if the xdebug_config server variable is set, if that is the case, the session will receive the xdebug cookie allowing the debugger to work.
I have included internations/http-mock to mock a php webserver, we could update the other feature to use this as well. But let's handle that separate.Travis is failing because internations/http-mock requires php7.. I'll see if I can test this in a different way.Http mocking was not needed, still it is weird to use an online domain to do this.
Suggestions are welcome.