Skip to content
This repository was archived by the owner on Jan 9, 2025. It is now read-only.

Add support for zombie options#183

Merged
aik099 merged 7 commits intominkphp:masterfrom
berliner:berliner/zombie-options
Jul 18, 2017
Merged

Add support for zombie options#183
aik099 merged 7 commits intominkphp:masterfrom
berliner:berliner/zombie-options

Conversation

@berliner
Copy link
Copy Markdown
Contributor

This is a follow up of #154.

This PR allows to specify zombie options as an array argument to the server constructor.
I have applied more or less the same logic as for the threshold argument.
Tests have been updated/added too.
I didn't update the README, but instead would issue a PR against the docs repo once this PR here has been approved.
The associated PR for Behat/MinkExtension is Behat/MinkExtension#284

Comment thread src/NodeJS/Server.php
* @throws \LogicException When server is already running.
*/
public function setOptions($options)
{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use a typehint instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread src/NodeJS/Server.php

/**
* @var array
*/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please use private visibility for the new property (as it has both a getter and a setter which are public, there is no benefit making this accessible through inheritance, and protected stuff makes it harder to handle backward compatibility)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@stof I tried to follow what's already there and all the other properties are protected while having setters and getters.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@berliner , please make this change. I guess that's last thing before this PR is merged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aik099 Done, though I still feel that this doesn't belong into this PR.

@aik099
Copy link
Copy Markdown
Member

aik099 commented Jul 18, 2017

Merging, thanks @berliner .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants