Builder fluent interface is confusing

[Slamdunk]

Currently the Token Builder is confusing:

jwt/src/Token/Builder.php

Lines 76 to 91 in 602856e

	public function withClaim(string $name, mixed $value): BuilderInterface
	{
	if (in_array($name, RegisteredClaims::ALL, true)) {
	throw RegisteredClaimGiven::forClaim($name);
	}
	return $this->setClaim($name, $value);
	}
	/** @param non-empty-string $name */
	private function setClaim(string $name, mixed $value): BuilderInterface
	{
	$this->claims[$name] = $value;
	return $this;
	}

The name of the method resemble an immutable object, but under the hood it's a fluent interface.
All my colleagues get confused too by reading a method like:

class User
{
    public function fillJwtToken(JwtBuilder $jwtBuilder): void
    {
        $jwtBuilder->withClaim('user-id', $this->id);
    }
}

Did the developer forget to return the new object?
Or its more a setClaim rather than a withClaim?

Can we take a direction and forget the other one?

[lcobucci]

I feel like the API should be immutable but it's a BC-break (interface change to return a value) that brings so little benefit now...

I don't mind changing it for v6, though. I want to ship v5 ASAP.

[Slamdunk]

I feel like the API should be immutable

After a day, I think the opposite: it's not an Entity nor a VO, it's a Builder, so it can be mutable.

It's just the nomenclature and return type that are misleading.
Moving everything to (): void should be enough to explicit its nature.

I don't want to rush anything, but please take a minute to think about this:

diff --git a/src/Builder.php b/src/Builder.php
index 63cac11..e4e6089 100644
--- a/src/Builder.php
+++ b/src/Builder.php
@@ -18,50 +18,50 @@ interface Builder
      *
      * @param non-empty-string ...$audiences
      */
-    public function permittedFor(string ...$audiences): Builder;
+    public function permittedFor(string ...$audiences): void;

     /**
      * Configures the expiration time
      */
-    public function expiresAt(DateTimeImmutable $expiration): Builder;
+    public function expiresAt(DateTimeImmutable $expiration): void;

     /**
      * Configures the token id
      *
      * @param non-empty-string $id
      */
-    public function identifiedBy(string $id): Builder;
+    public function identifiedBy(string $id): void;

     /**
      * Configures the time that the token was issued
      */
-    public function issuedAt(DateTimeImmutable $issuedAt): Builder;
+    public function issuedAt(DateTimeImmutable $issuedAt): void;

     /**
      * Configures the issuer
      *
      * @param non-empty-string $issuer
      */
-    public function issuedBy(string $issuer): Builder;
+    public function issuedBy(string $issuer): void;

     /**
      * Configures the time before which the token cannot be accepted
      */
-    public function canOnlyBeUsedAfter(DateTimeImmutable $notBefore): Builder;
+    public function canOnlyBeUsedAfter(DateTimeImmutable $notBefore): void;

     /**
      * Configures the subject
      *
      * @param non-empty-string $subject
      */
-    public function relatedTo(string $subject): Builder;
+    public function relatedTo(string $subject): void;

     /**
      * Configures a header item
      *
      * @param non-empty-string $name
      */
-    public function withHeader(string $name, mixed $value): Builder;
+    public function withHeader(string $name, mixed $value): void;

     /**
      * Configures a claim item
@@ -70,7 +70,7 @@ interface Builder
      *
      * @throws RegisteredClaimGiven When trying to set a registered claim.
      */
-    public function withClaim(string $name, mixed $value): Builder;
+    public function withClaim(string $name, mixed $value): void;

     /**
      * Returns a signed token to be used

Is this the best value we can provide as a library? I don't know.
Does this convey better the nature of the underlying implementations? IMHO yes.

[Ocramius]

IMO no, a builder can be immutable, and if something can be immutable, it's a good idea for it to be.

From an API PoV, the : void suggestion increases verbosity, IMO.

Meanwhile, if the builder becomes immutable, a missed call to the builder (a "not consumed" builder) will result in Psalm reporting an unused value 👍

EDIT: also, I wouldn't want a mutable builder to be re-used, even by accident, as a service.