allow method chaining

[browner12]

Target branch: Probably a new major, with the interface change?
Resolves issue NA

• It is a Bug fix
• It is a New feature
• Breaks BC
• Includes Deprecations

Allow users to chain methods when building their OTP.

I believe this breaks BC, and warrants a new Major due to the interface change.

[Spomky]

Hi,

Many thanks for this PR.
This is a nice feature. Let me see how to manage the changes regarding the BC break.
I could take the opportunity to drop PHP 8.1 that entered in the security fix only release phase.

[browner12]

any updates?

[Slamdunk]

Method chaining is ok, but no fluent interfaces please as discussed in #166 (comment)

I'm going to propose a PR against v11 that:

1. Introduces withXxx methods where a new instance is returned every time
2. Deprecates setXxx methods
3. Marks every class as @readonly

In v12 then we can drop deprecated classes, and mark classes as readonly with native PHP statement

[Slamdunk]

For reference, this is what we've done in lcobucci/jwt package:

1. New APIs and soft deprecations in next minor: Add migration API for readonly classes lcobucci/jwt#1092
2. BC Breaks in next major: Mark all classes as readonly lcobucci/jwt#1091

[browner12]

Method chaining is ok, but no fluent interfaces please

I'm confused, those are the same thing.

I've read through your other PR and that 12 year old article from Ocramius, and I must be missing something because it makes no sense.

You're gonna have to ELI5, because I'm not seeing what the problem is with this incredibly common pattern.

[Slamdunk]

An object that can mutate state is unpredictable as the same call to one of its API can return different results.
Immutable objects are much easier to work with, as the same call always returns the same result.

The static return type is ok.

The return $this; statement is the problem.

Ideally the next major version should come with all the objects marked as readonly, and so the only accepted return statements are something like return new MySelf($newVar);

[Spomky]

Hi,

Time goes by, and I must admit I totally forgot what I wrote three years ago 😅
So, to keep things simple: I don’t want to introduce clone or add extra complexity with new withXxx methods.

I understand that method chaining is common (especially with builders) but in this case, I believe the default values are perfectly fine. I don't see any real issue with calling $otp->setXxx(); multiple times when specific adjustments are needed.

Thanks again for the proposal and the discussion!

[Slamdunk]

I don’t want to introduce clone

May I ask you why?

readonly classes are so worth it for security critical applications 💣

[browner12]

Mutable vs immutable is a whole separate discussion outside the scope of this PR.

These classes are already mutable, and this PR does nothing to change that.

Returning $this is basically a zero-risk enhancement. It allows people to use chaining, but does not force them to.

With this PR, users can choose to use either:

$totp = Totp::generate();
$totp->setPeriod(10);
$totp->setEpoch(10);

or

$totp = Totp::generate()
    ->setPeriod(10)
    ->setEpoch(10);

based on their personal preference.

[Slamdunk]

Mutable vs immutable is a whole separate discussion outside the scope of this PR.

These classes are already mutable, and this PR does nothing to change that.

Major versions with BC Breaks should be minimized.

The fact that this PR is zero-risk enhancement doesn't mean much: a BC Break is needed, so it's better to discuss all the BC Breaks that we want to apply to avoid another Major version in a near future.

The immutability topic is inside the scope of the PR, because if accepted this very PR becomes invalid.

[browner12]

Agree to disagree.

I've proposed a simple change that will allow method chaining (very common pattern). The breaking change is likely non-existent, as it would only apply to users who created their own classes that implement the interface.

@Slamdunk if you want to argue for a major shift in this codebase to immutability, I'd suggest you do it in a separate PR as it would likely also include scope outside of this PR. If you were to somehow convince a switch to immutability, it doesn't make this PR invalid, it just means the chaining needs to adjust to immutable chaining.

@Spomky can you either merge or close?

[Slamdunk]

#244 is my proposal for a fluent API, ready to be upgraded to readonly classes in the next major.