Server options as constructor argument

[basz]

Before I continue, do you think approach is correct?

IMO this is ok... But looking at the test, it is possible to change behaviour by mutating the ServerOptions. When we do this everywhere that might lead to unexpected results?

[coveralls]

Coverage decreased (-0.3%) to 75.975% when pulling 073f6c1adf68c24737803417aa4f08d5572ccd4e on basz:server-options-as-constructor-argument into a183c35ec8355dbcd397ff6573699d91fb7b148c on zf-fr:master.

[basz]

@ojhaujjwal

Why not just pass that option as a constructor argument?

This ok by you?

cc/@bakura10

[ojhaujjwal]

@basz thanks for tagging. looks great :+1:

[danizord]

@basz since we're breaking BC anyway and now ServerOptions instance is stored everywhere, I think it would be nice to make ServerOptions immutable to avoid side effects.

[ojhaujjwal]

@danizord agreed. setters are a big NO.

[bakura10]

+1

[coveralls]

Coverage increased (+0.3%) to 76.556% when pulling 2a4f5920759ceb74b234597062a535b2950532c5 on basz:server-options-as-constructor-argument into a183c35ec8355dbcd397ff6573699d91fb7b148c on zf-fr:master.

[basz]

• done •

[basz]

@bakura10 this what yo mean?

plus a fail; but I don't see why; tests/src/Options/ServerOptionsTest.php:64

[coveralls]

Coverage increased (+0.4%) to 76.701% when pulling 776e57e1485d9f7e65610f26459deae438dbb5f0 on basz:server-options-as-constructor-argument into a183c35ec8355dbcd397ff6573699d91fb7b148c on zf-fr:master.

[coveralls]

Coverage increased (+0.4%) to 76.701% when pulling f5222ad16c96d2ff76ec08b4a5ecac415b63d7d2 on basz:server-options-as-constructor-argument into a183c35ec8355dbcd397ff6573699d91fb7b148c on zf-fr:master.

[basz]

@bakura10 @danizord I'm not sure I like the current implemetation of ServerOptions very much.

Main point; It introduces a dependency for a problem that isn't there. Bloats; The class is lightweight enough for a few setters, which when private still ensure immutability. (class is also marked final). Now we I check if a option is set twice... And get awkward checks for combinations that the assertion library doesn't provide for (null or callable or string). Private setters are more expressive and guard values in logical way. Bahavior; With private setters it is still immutable.

The named constructor does seem helpful, but I think it would be more elegant to keep the setters in this case?

[bakura10]

Actually as suggested @danizord an alternative to remove teh dependency to Assertion would be to do that:

private function construct(
   bool $param1,
   int $param2
) {
   $this->param1 = $param1;
   $this->param2 = $param2;
}

static public function fromArray(array $options)
{
   return new self(
     $options['param1'] ?? true,
     $options['param2'] ?? 400
   );
}

Therefore you keep the built-in typehinting and provide true immutability. What do you think?

[basz]

I think that's better - (until (many) more options are added)

still don't see the 'true immutability' argument. private setters provide true mutability...

I will refactor...

[coveralls]

Coverage increased (+0.04%) to 76.31% when pulling d660a0a1d0b10010c81cba3fc8562e35d257a177 on basz:server-options-as-constructor-argument into a183c35ec8355dbcd397ff6573699d91fb7b148c on zf-fr:master.

[danizord]

@basz the point of using constructor instead of private setters is to ensure that an object is always in consistent state after construction.

[basz]

@danizord i get that, just saying that a mega constructor is not always nice.

i tend to create ValueObjects with the following method

static withSomething : returns mutated copy) private setSomething : from (named) constructors, may do guarding public getSomething : get value

[danizord]

I see, so it's a matter of preference. I think setters are more verbose and PHPStorm generates the constructor automatically anyway :P