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

Support zombie.js options #154

Closed
robballou wants to merge 7 commits into from
Closed

Support zombie.js options #154

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a17347
Started on allowing Zombie.js options
robballou Feb 2, 2016
f57a58e
Merge branch 'master' into zombie-options
robballou Feb 3, 2016
860aaca
Removed option validation, moved options methods, added getOption() w…
robballou Mar 4, 2016
ab2a51e
Added createTemporaryServer() call in setOptions()
robballou Mar 4, 2016
80999d8
Removed setOption(), updated setOptions()
robballou Mar 4, 2016
95fa786
Removed getOption()
robballou Mar 4, 2016
847ecea
Updated setOptions() code styles
robballou Mar 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion src/NodeJS/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ abstract class Server
*/
protected $connection;

/**
* @var array
*/
protected $options = array();

/**
* Constructor
*
Expand All @@ -76,7 +81,8 @@ public function __construct(
$nodeBin = null,
$serverPath = null,
$threshold = 2000000,
$nodeModulesPath = ''
$nodeModulesPath = '',
$options = array()
) {
if (null === $nodeBin) {
$nodeBin = 'node';
Expand All @@ -93,6 +99,8 @@ public function __construct(

$this->serverPath = $serverPath;
$this->threshold = intval($threshold);

$this->setOptions($options);
}

/**
Expand Down Expand Up @@ -253,6 +261,52 @@ public function getConnection()
return $this->connection;
}

/**
* Return the options.
*
* @return array
*/
public function getOptions()
Copy link
Copy Markdown
Member

@aik099 aik099 Feb 4, 2016

Choose a reason for hiding this comment

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

Looking at order of existing setters/getters shows that all getters are grouped into one group, and all setters are grouped into another group. Could you please move your setter/getter to comply with that?

{
return $this->options;
}

/**
* Set options array.
*/
public function setOptions($options)
Copy link
Copy Markdown
Member

@aik099 aik099 Feb 4, 2016

Choose a reason for hiding this comment

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

Please update example in README.md to show that it's now possible to set Zombie options.

While we're at it you can send PR's to MinkExtension and PHPUnit-Mink to update Zombie integration to allow specifying Zombie options in there as well.

That would be a great addition.

Copy link
Copy Markdown
Member

@stof stof Feb 16, 2016

Choose a reason for hiding this comment

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

Rather than updating the readme, it would be better to update the docs repo though

{
// pass these to the setOption() method which will validate.
foreach ($options as $key => $value) {
$this->setOption($key, $value);
}
}
Copy link
Copy Markdown
Member

@aik099 aik099 Mar 4, 2016

Choose a reason for hiding this comment

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

Please add following code afterwards:

$this->serverPath = $this->createTemporaryServer();

to regenerate server code.


/**
* Set a single option.
*/
public function setOption($option, $value)
Copy link
Copy Markdown
Member

@aik099 aik099 Mar 4, 2016

Choose a reason for hiding this comment

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

Please inline this method (after fixing other snuff), because it will complicate things down the road.

Copy link
Copy Markdown
Author

@robballou robballou Mar 4, 2016

Choose a reason for hiding this comment

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

Not sure I follow what this means. I just posted some other changes, so let me know and I'll get that in there.

Copy link
Copy Markdown
Member

@aik099 aik099 Mar 4, 2016

Choose a reason for hiding this comment

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

Inline means:

  1. put body the method (setOption in this case) inside the method, that calls it (setOptions in this case)
  2. remove original method (setOption in this case)

{
$valid_options = array(
Copy link
Copy Markdown
Member

@aik099 aik099 Feb 4, 2016

Choose a reason for hiding this comment

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

You can define this once as class property instead of defining this array each option set.

Copy link
Copy Markdown
Member

@aik099 aik099 Feb 4, 2016

Choose a reason for hiding this comment

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

Actually better not to validate any options at all, because:

  • in future Zombie versions new options could be added and driver won't be able to set them unless we update it each time
  • we don't validate $value and for example developer can pass string instead of boolean and Zombie would fail anyway

'features',
'headers',
'waitDuration',
'proxy',
'referrer',
'silent',
'site',
'strictSSL',
'userAgent',
'language',
'runScripts',
'localAddress',
Copy link
Copy Markdown
Member

@stof stof Mar 4, 2016

Choose a reason for hiding this comment

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

these option are specific to ZombieServer, so they should not be in this class

Copy link
Copy Markdown
Member

@aik099 aik099 Mar 4, 2016

Choose a reason for hiding this comment

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

I've said the same in above comment.

);

if (in_array($option, $valid_options)) {
Copy link
Copy Markdown
Member

@aik099 aik099 Feb 4, 2016

Choose a reason for hiding this comment

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

That's not normal case, when non-supported option is given, so better to throw an exception immediately.

$this->options[$option] = $value;
}
}

/**
* Starts the server process
*
Expand Down Expand Up @@ -423,6 +477,7 @@ protected function createTemporaryServer()
'%host%' => $this->host,
'%port%' => $this->port,
'%modules_path%' => $this->nodeModulesPath,
'%options%' => json_encode($this->options),
));

$serverPath = tempnam(sys_get_temp_dir(), 'mink_nodejs_server');
Expand Down
2 changes: 1 addition & 1 deletion src/NodeJS/Server/ZombieServer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ protected function getServerScript()

stream.on('end', function () {
if (browser == null) {
browser = new zombie();
browser = new zombie(%options%);

// Clean up old pointers
pointers = [];
Expand Down