Add parameters to Server object

[aledeg]

I added the parameters in the server class to support what we talked about in #18 and #19.
I didn't add it in the script that initialize the server since I don't know how to deal efficiently with the parameters.
If I find something clever before you do, I will update my PR.

[aledeg]

I've added a couple of thing in that PR. Now, it's supposed to read parameters from CLI and change the server behavior accordingly.

[ddelnano]

Ok I will take a look at this later today when I get the chance.

[ddelnano]

@aledeg I am guessing this foreach loop is used because $argv is passed in the Hooks::loadHooks???

[aledeg]

@ddelnano Yep. There is no easy way to do it. Or at least, I am not aware of one if it exists.

I figured it wasn't a big deal since it is done only once during the server initialization process.

[ddelnano]

@aledeg I am thinking it might be cleaner to parse the hooks like you did for the other cli options / flags and then change Hooks::loadHooks($hooks) rather than passing the entire argv variable in.

What do you think about that? It was honestly a mistake that I wrote it to accept $argv I should have originally parsed it from the flags.

[aledeg]

@ddelnano For sure, it will be better and probably testable. I will look at it tonight.

Do you know how dredd starts the server? Does it append the hookfiles property after the language property? So if you happen to have

# dredd.yml
hookfiles: /path/to/hooks
language: bin/dredd-hooks-php

does it trigger the server like?

bin/dredd-hooks-php /path/to/hooks

If so it would be a good idea to have the script running like this to separate options from input

bin/dredd-hooks-php --host 0.0.0.0 --port 12345 --force -- /path/to/hooks

[aledeg]

I can confirm that dredd launch the following:

php bin/dredd-hooks-php /path/to/hooks

[ddelnano]

@aledeg I am thinking it just passes it the path directly otherwise my $argv variable in Hooks::loadHooks would fail. But I would need to test this to verify. Its been a while since I have that about that.

[aledeg]

@ddelnano So I guess, it will be easier to parse the data on the input. I will look into it tonight. I'll keep you posted

[aledeg]

@ddelnano So I simplified the code I wrote. It turns out your method of parsing the arguments was good enough to discard extra arguments. So I reverted my code to simplify it and changed variables names to reflect their content.

Let me know if I should do something more.

[ddelnano]

@aledeg awesome, yea looks much cleaner. The only suggestion I have is I am not a huge fan of setters such as Server::setHost and Server::setPort in this instance.

I would rather have the constructor accept both of those arguments (host and port) and then remove the default values like so. To me, it makes sense that when a "server" is created it should already have its host and port. And for the life of the server they should not change. Could you update your PR with the following and then ill be happy to merge it :)

...
private $host;
private $port;

public function __construct($host = '127.0.0.1', $port = 61321)
{
  $this->host = $host;
  $this->port = $port;
  $this->runner = new Runner;
}

Then in the bin/dredd-hooks-php file

$host = array_key_exists('host', $options)
             ? $options['host']
             :  '127.0.0.1';
$port = array_key_exists('port', $options)
             ? $options['port']
             :  '127.0.0.1';
 $server = new Server($host, $port);

[aledeg]

@ddelnano good idea! I think also it's a cleaner way. I didn't think of that yesterday :)

[aledeg]

@ddelnano it passed CI. I think it's good now :)

[ddelnano]

👍 just gonna give it one last look over really quick.

[ddelnano]

Cool merging now. Awesome work!

[ddelnano]

@aledeg I am on mobile now but will tag a new release when I get in front of a computer.