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

Fixing config.yml param "bin_path" - bin path is now undepending on t… #6

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

linslin
Copy link
Contributor

@linslin linslin commented Mar 9, 2017

An other windows fix, could you please check if this still works on unix? - linslin/Yii2-Curl#57

@mcustiel
Copy link
Owner

mcustiel commented Mar 9, 2017

Hey @linslin, thank you for your help with this project.
There is a merge conflict, could you merge my master to your fork?

@linslin
Copy link
Contributor Author

linslin commented Mar 9, 2017

Oh yea, you are right. Let me try again.

@linslin
Copy link
Contributor Author

linslin commented Mar 9, 2017

Allright, conflict should be solved.

@@ -47,6 +47,7 @@ class PhiremockProcess
*/
public function start($ip, $port, $path, $logsPath, $debug)
{
$path = realpath($path);
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about adding the call to realpath when the start method is being called in Phiremock.php file? That way, this method is agnostic about how the path is built.
Would also be great to call it for logsPath variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutly, we thought about the logFilePathe too but we needed a quick fix for the bin_path first. You are more familiar with your codes, so please decide it by yourself. =)

Copy link
Owner

Choose a reason for hiding this comment

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

Ok. Will merge it and later move it outside the method also for logspath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thx m8. Followed you up =)

@mcustiel mcustiel merged commit 5d8f628 into mcustiel:master Mar 9, 2017
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.

2 participants