php-mqtt/client

ConnectionSettings setter methods modify copy of object

heyrex opened this issue · 3 comments

Each setter method inside ConnectionSettings returns a clone of $this with the new setting applied, but the original object remains unmodified. This is makes certain normal programming practices very syntactically awkward.
Eg: Conditionally changing a single setting using a one line if statement becomes if($foo) $settings = $settings->setSomeSetting(true);

Is there some technical reason why ConnectionSettings should clone itself then modify the clone? If this is an oversight, I would be happy to submit a pull request to fix this peculiar behaviour.

The fluent interface in combination with an immutable object is intended behavior and by design. The primary reason for this design decision is to reduce the complexity of the MqttClient class, because the settings passed to the connect($settings) method cannot be altered while the client is connected.

In your case, I see two solutions:

  1. A solution which already works is to pass the value by condition instead of performing the call by condition:

    $settings = (new ConnectionSettings)
        ->setSomeSetting($foo)                  // if $foo is a boolean
        ->setSomeSetting($foo ? true : false);  // if $foo is not a boolean
  2. A solution which involves adding one or two new convenience methods (which I would be willing to add):

    $settings = (new ConnectionSettings)
        ->when($foo, fn ($s) => $s->setSomeSetting(true))
        ->unless($foo, fn ($s) => $s->setSomeSetting(false));

Thanks for the response on this. The fluent interface does improve readability, it's the immutability behaviour which is unexpected. On re-reading, I see this is mentioned in README.md. Perhaps this can be made clear in the class documentation? Naming the class ImmutableConnectionSettings might also help someone who is troubleshooting.

These comments would have been helpful in my situation: #145

Some alternatives consider:

  1. Have the MqttClient->connect() method create the clone, thus the fluent behaviour would be 100% standard and support all programming styles.
  2. Self documenting code: Make MqttClient->connect() require ImmutableConnectionSettings and add a compatibility overload for connect(ConnectionSettings, ...).

I have seen a number of implementations using php-mqtt which have long variable definition blocks and flow control blocks before instantiating ConnectionSettings, which makes me think the immutability is defeating the fluent goal of simplicity/readability.

Happy to close.

Thanks for the PR, adding some hints in the code doesn't seem like a bad idea. 👍

Regarding alternatives, the best approach would be a ConnectionSettingsBuilder which is not immutable and builds the immutable (and read-only) ConnectionSettings with ConnectionSettingsBuilder::build():

$builder = (new ConnectionSettingsBuilder)
    ->setUsername('myapp');

if ($password) {
    $builder->setPassword($password);
}

$connectionSettings = $builder->build();
$mqtt->connect($connectionSettings);

Since this is a breaking change, I'm not really in favour of changing this at the moment though. There haven't been any other reports of misunderstandings so far either. But I'll keep an eye on it for future (major) releases.